As part of the interface refactoring, we want to implement a cardForProperty function (see url). This can be on nsIAbDirectory in the first instance if nsIAbCollection doesn't exist. There is already a mork-specific implementation on nsIAddrDatabase.

Here's a version without any tests. I'm holding off on reviews until I get tests whipped up.

And this is where I talk about the wonder of tests. It turns out that nsAddrDatabase didn't actually implement the property correctly: its case-insensitive search was "make it lower case and then do case-sensitive a search," which my new tests caught. Also the TestMorkReader file from bug 424446 works wonders for reading .mab files. Instructions are included in the README in db/morkreader.

Comment on attachment 334202 [details] [diff] [review] Full patch, v. 1 >+ >+ /** >+ * Returns the address book card for the specified property if found. >+ * >+ * If the address book type does not know how to do this, it will throw >+ * NS_ERROR_NOT_IMPLEMENTED. Core address book types do throw this exception, >+ * so beware when calling this method. >+ * >+ * If the property is not natively a string, it can still be searched for >+ * using the string-encoded value of the property, e.g. "0". I'm a bit worried about that statement. On the OS X directory (for instance) I just have a list of nsIAbCards. Supporting searching for numbers is probably going to be difficult, or will nsAbCardProperty do the conversion for me? >+ * >+ * @param aProperty The property to look for. >+ * @param aValue The value to search for. >+ * @param aCaseInsensitive True if matching should be done case-insenstively. >+ * @result An nsIAbCard if one was found, else returns NULL. >+ */ >+ nsIAbCard cardForProperty(in string aProperty, in AUTF8String aValue, >+ in boolean aCaseInsensitive); > }; The spec actually has aCaseSensitive, and I think we should implement it that way, its slightly more logical when you are looking at parameters and trying to work out what is going on. I'm not sure if we want/need to change nsIAddrDatabase and co, but certainly for new stuff I'd like it the logical way around. >+ >+ // Passing into this function is already converted to lower case >+ if (aCaseInsensitive) >+ ToLowerCase(columnValue); >+ > if (NS_SUCCEEDED(rv) && columnValue.Equals(unicodeStr)) Why not just pass CaseInsensitiveCompare (or the other one) to the Equals? (If you can support internal/external api like I did in attachment 334095 [details] [diff] [review] that would be useful as well). >+function check_correct_card(card) { >+ do_check_true(card != null); nit: I prefer this to be do_check_neq(card, null); >+ // Check cardForProperty returns null correctly for non-extant properties >+ do_check_true(AB.cardForProperty("JobTitle", "", true) == null); >+ do_check_true(AB.cardForProperty("JobTitle", "JobTitle", true) == null); >+ ... >+ >+ do_check_true(AB.cardForProperty("JobTitle", "jobtitle1", false) == null); nit: ditto for these cases (except use do_check_eq). I find them slightly easier to understand quickly if they fail.

Changes: 1. Fixed some whitespace in nsAddrDatabase::GetCharColumnForAttribute 2. Clearer case-insensitivity. 3. Clarified documentation. 4. aCaseInsensitive -> aCaseSensitive.

Comment on attachment 334277 [details] [diff] [review] Full patch, v. 2 Looks good. Thanks for the patch, and sorry for the review delay on my end. Some minor stuff: > /** >- * Returns the address card for the specified email address if found. >+ * Returns the address book card for the specified email address if found. > * > * If the address book type does not know how to find a card, it will throw > * NS_ERROR_NOT_IMPLEMENTED. Core address book types do throw this exception, > * so beware when calling this method. > * > * @param emailAddress The email address to find in either the primary or > * secondary email address fields. If email address is > * empty, the database won't be searched and the function > * will return as if no card was found. > * @return An nsIAbCard if one was found, else returns NULL. > */ > nsIAbCard cardForEmailAddress(in AUTF8String emailAddress); >+ >+ /** >+ * Returns the address book card for the specified property if found. I think both this comment and the one for cardForEmailAddress should explicitly describe what behavior will happen if there are two cards in one directory that match the given criteria (might just be "returns some card, implementation defined which one"). >+ * If the address book type does not know how to do this, it will throw >+ * NS_ERROR_NOT_IMPLEMENTED. Core address book types do throw this exception, >+ * so beware when calling this method. This wants to be in an @exception block, I bet. >+ * If the property is not natively a string, it can still be searched for >+ * using the string-encoded value of the property, e.g. "0". See >+ * nsIAbCard::getPropertyAsAUTF8String for more information. >+ * >+ * @param aProperty The property to look for. >+ * @param aValue The value to search for. It's probably worth specifically documenting what happens when an empty value is passed in here. >+ * @param aCaseSensitive True if matching should be done case-senstively. >+ * @result An nsIAbCard if one was found, else returns NULL. >+ */ >+ nsIAbCard cardForProperty(in string aProperty, For people new to the code base reading code that calls this, getCardFromProperty seems like it would be more intuitively obvious. It would be worse for folks writing code attempting to intuit back and forth between cardForEmail and this method. Still, approachability seems like something that we desperately need more of, so I think we should make the change (assuming Standard8 is ok with it). Maybe we should also just change cardForEmail similarly. Why |string| rather than |ACString|? Remember to add your info to the license boilerplate of files you touch. sr=dmose with the above points addressed as appropriate.

(In reply to comment #5) > For people new to the code base reading code that calls this, > getCardFromProperty seems like it would be more intuitively obvious. It would > be worse for folks writing code attempting to intuit back and forth between > cardForEmail and this method. Still, approachability seems like something that > we desperately need more of, so I think we should make the change (assuming > Standard8 is ok with it). Maybe we should also just change cardForEmail > similarly. No you mention it, I think getCardFromProperty does sound better. I'd be happy for cardForEmail to change as well, but let's get this patch in first.

I did the documentation changes and changed cardForProperty -> getCardFromProperty. Checked in as changeset 63cb145ce16e.

Something that came up in discussion at MoMo yesterday is that probably both of these APIs want to really return multiple objects, rather than just one.