Per bug 450149 comment 8, we should extend these apis to return multiple cards. The results could then be used to provide matches where two people use the same email address, which could then be handled appropriately by the caller.

(Mark, you copied the same function twice in the summary.) ***** IIRC with Mark, about bug 309057 comment 77, I had an idea that the |getCardForAddress()| there could return |[card, matching]|, using |matching| to tell if the provided function matched or if returning the first found card with the right email address only. Mark was concerned about possible inconsistencies on the card which would be returned/used (in different cases). I'm not sure either way atm, but I postpone this issue to this (later) bug :-|

(In reply to comment #1) > I'm not sure either way atm, but I postpone this issue to this (later) bug :-| This bug is only for the API changes to the address book.

It feels important for extension authors to have some way to get this data in a Tb3 timeframe. It could be a modification of these APIs or some other API.

Talking to sid on irc today I've realised that this functionality is already available within the address book, though its not easy access. In summary: You can set up a search uri on an address book for one attribute, then use .childCards to get the cards. This is more expensive (cpu/time) than a getCardFromProperty equivalent function because it gets all the cards from the database into nsAbCardProperty objects and then filters them - but the function is there. Slightly extended description: - Get an nsIAbDirectory URI - Extend URI with search parameters e.g. "(or(PrimaryEmail,bw,@V))" - Get a new nsIAbDirectory with the extended URI via nsIAbManager.getDirectory. - Call .childCards for that nsIAbDirectory. So I'm not sure we need to block on this because extensions do have a way to do it. Thoughts?

That's a reasonable argument, I have to admit, though as you say, it's a pretty ugly workaround. I've added a few extension authors to get their thoughts on this: folks, is there stuff you couldn't/wouldn't do in your extensions if the workaround Mark describes is the only API available for this functionality?

I won't say I fully understand the suggested workaround but assuming it's performance/resource consumption is no worse than tb2's database.getCardFromAttribute() I can't see it being an issue for zindus. What would be really nice is if there were some way of doing O(1) lookup of cards. If uuid: https://bugzilla.mozilla.org/show_bug.cgi?id=444093 offered that then zindus would probably switch and not need this kind of lookup at all. In terms of sizing a test case, Google has a limit of 10,000 contacts per addressbook and some users go close to that.

I'm starting to think we do actually want a fast function such as this - although it means another function in nsIAddrDatabase, its the sort of thing we want to be able to do. For instance it would make searching the AB for the second email address easier and faster which is something I'd like to be able to do for bug 447927.

Unassigning myself from this for now in the hopes that someone can pick it up. I'd certainly like to get this option in for TB 3.

Standard8 suggested that a reasonable API here would be to simply duplicate the two existing method, but pluralize them and have them return nsISimpleEnumerators. This sounds like a good plan to me.

Sounds good. I'll take the bug.

It's been a few weeks, so I wanted to show some progress. I am removing the added member variable m_rowPos in the final patch. If you're brave and take a look, you might miss the part where I |#if 0| a huge part of code which makes the patch work, but I don't know the consequences of removing that code, yet.

(In reply to comment #11) > It's been a few weeks, so I wanted to show some progress. I am removing the > added member variable m_rowPos in the final patch. If you're brave and take a > look, you might miss the part where I |#if 0| a huge part of code which makes > the patch work, but I don't know the consequences of removing that code, yet. The primary consequence is that you always take the slow path in search. I don't know enough about mork guarantees of how FindRow works under multiple results to know if this is possible to overcome. I think, for performance reasons, it would be helpful to have the single-return do the O(1) lookup if possible, which would mean conditionally executing the if statement.

I added a few optional parameters to some functions that lets you keep track of the mork db position, so you can make multiple calls to the function. Other than that, it seems pretty straightforward. And there are tests and comments!

One little nit: could you add documentation to the IDL functions?

Comment on attachment 386369 [details] [diff] [review] first attempt This is a good first patch and I like the method of implementation. A few comments though: >diff --git a/mailnews/addrbook/public/nsIAbCollection.idl b/mailnews/addrbook/public/nsIAbCollection.idl You need to bump the uuid of nsIAbCollection and nsIAddrDatabase. "/msg firebot uuid" on irc to get new numbers. >+ nsISimpleEnumerator getCardsFromProperty(in string aProperty, >+ in AUTF8String aValue, >+ in boolean aCaseSensitive); ... >+ nsISimpleEnumerator getCardsFromAttribute(in nsIAbDirectory aDirectory, >+ in string aName, >+ in AUTF8String uUTF8Value, >+ in boolean aCaseInsensitive); As Joshua said, please document these interfaces in the same style as their original versions. >+ >+ > PRBool findMailListbyUnicodeName(in wstring listName); No need for two blank lines here, one will do fine. >+NS_IMETHODIMP nsAbMDBDirectory::GetCardsFromProperty(const char *aProperty, >+ const nsACString &aValue, >+ PRBool caseSensitive, >+ nsISimpleEnumerator **result) >+{ ... >+ nsresult rv; >+ if (!mDatabase) >+ { >+ rv = GetAbDatabase(); >+ if (rv == NS_ERROR_FILE_NOT_FOUND) >+ return NS_OK; >+ NS_ENSURE_SUCCESS(rv, rv); >+ } nit: please put nsresult rv inside the if statement and assign the GetAbDatabase result straight to it on initialisation. >+NS_IMETHODIMP nsAddrDatabase::GetCardsFromAttribute(nsIAbDirectory *aDirectory, >+ const char *aName, >+ const nsACString & aUTF8Value, >+ PRBool aCaseInsensitive, >+ nsISimpleEnumerator **cards) >+{ ... >+ nsresult rv; >+ nsCOMPtr<nsIMutableArray> list = do_CreateInstance(NS_ARRAY_CONTRACTID, &rv); Well you should check for success on creation. However, please drop these two lines and use nsCOMArray<nsIAbCard> instead - it saves the xpcom overhead as we aren't returning the array anywhere. >+ while (!done) May as well make this a do...while loop as the condition would always be false on entering. Please also remove the extra whitespace from the end of the line. >+ return list->Enumerate(cards); It is better to use NS_NewArrayEnumerator from nsArrayEnumerator.h. This can be used with nsIArray or nsCOMArray. >+ rowPos = -1; Might as well do this initialisation where it is declared. >- m_mdbPabTable->GetTableRowCursor(m_mdbEnv, -1, getter_AddRefs(rowCursor)); >+ m_mdbPabTable->GetTableRowCursor(m_mdbEnv, rowPos , getter_AddRefs(rowCursor)); nit: no space before the comma. >diff --git a/mailnews/addrbook/test/unit/data/cardForEmail.mab b/mailnews/addrbook/test/unit/data/cardForEmail.mab This change breaks test_nsAbAutoCompleteSearch1.js, as test_nsAbAutoCompleteSearch1.js effectively borrowed the file, I suggest you hg copy it and do the modification on the original file. The unmodified copied file can then be used for test_nsAbAutoCompleteSearch1.js.

Thanks for the review Mark. I made the changes you suggested. Holding off on asking review until it's not 3am and I look at my code again.

Comment on attachment 391805 [details] [diff] [review] nits fixed, tests pass Sorry for the delay in getting back to this. >+ /** >+ * Returns all address book cards with a specific property matching value ... >+ * @result A nsISimpleEnumerator that holds nsIAbCard >+ * instances. May be null under certain curcumstances >+ */ >+ nsISimpleEnumerator getCardsFromProperty(in string aProperty, >+ in AUTF8String aValue, >+ in boolean aCaseSensitive); nsISimpleEnumerator can't be null except in error conditions. As error conditions throw, then we don't need to document the null fact here. r=Standard8 with that fixed.

Comment on attachment 391805 [details] [diff] [review] nits fixed, tests pass quick nit - curcumstances should be circumstances.

The Thunderbird drivers wish to release Thunderbird 3 as soon as possible. As a result, we feel that this bug shouldn't stand in the way of all the other good work getting into the hands of users sooner rather than later. Therefore we are retargeting it for 3.1. See http://ccgi.standard8.plus.com/blog/archives/242 for more details. The 3.1 release is expected to be a quick release soon after Thunderbird 3. (Note: we'd still like to get this in for TB 3 especially as it has a draft patch - we just won't block on it).

Updated for my comment. I think I have a use case that I'd really like to get in for TB 3 and having this capability would make it a lot easier to implement.

So implementing this I realise my use case can't use this, as it is an exact match rather than substring. However, given the patches are basically ready to go, I see no harm in taking these for TB 3.0.

Comment on attachment 404573 [details] [diff] [review] Fixed comment. should be an nsISimpleEnumerator + * @result Returns an SimpleEnumerator of nsIAbCard instances

Comment on attachment 404580 [details] [diff] [review] OS X implementation this looks OK, but test_abAutoCompleteSearch1.js failed on the mac with these patches, in the call to inputs.forEach(CheckInputSet), 8 == 6 failed. r/sr=me, once the test is sorted out.

Comment on attachment 404573 [details] [diff] [review] Fixed comment. Patch for new API, possibly unused in core, so low risk. a=Standard8.

Comment on attachment 404580 [details] [diff] [review] OS X implementation Low risk patch to go with the first implementing a new API. Based on existing code.

Both patches checked in: attachment 404573 [details] [diff] [review]: http://hg.mozilla.org/comm-central/rev/7c474098ace7 attachment 404580 [details] [diff] [review]: http://hg.mozilla.org/comm-central/rev/dfcd4aab7972 Thanks to Cesar for doing the bulk of this work.