452232 - Move LDAP autocomplete over to toolkit interfaces

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[Mark Banner (:standard8)]

Just like we've done for local address book autocomplete in bug 370306 we need to move LDAP autocomplete over to toolkit interfaces.

The proposal to do this:

- Implement a new service to interface directly to toolkit autocomplete, and provide a wrapper around the existing sessions.
- Move the identity set-up functions (see MsgComposeCommands.js) into the sessions, there is now a parameter which specifies which identity to use.
- The new service will load (and cache) a session for an identity.
- Note the sessions must be unloaded on shut-down (or potentially when the compose window is closed). This could be done via an extra interface to the new service.

Comment 1

[Mark Banner (:standard8)]

Attachment: WIP Patch

A very WIP version, still sorting out error handling, and xpfe autocomplete doesn't seem to like it as much as toolkit autocomplete.

Comment 2

[Mark Banner (:standard8)]

This isn't making beta 1... moving out.

Comment 3

[[e-mail]]

+    Components.classes["@mozilla.org/observer-service;1"]
+              .getService(Components.interfaces.nsIObserverService)
+              .notifyObservers(gMsgCompose, "compose-window-close", "");
What will we call this when we support LDAP completed address lists? ;-)

+  errorDescription: null,
IIRC this is what you're supposed to set when there's an error. The xpfe autocomplete should convert this internally into a fake (unselectable) result... I don't know about toolkit though.

+  // The cache is indexed by the address book URI. Each item in the cache
+  // then has three items:
+  // book - the address book associated with the URI.
+  // query - the nsAbLDAPDirectoryQuery in use for the URI.
+  // context - the search context for the URI (used for cancelling the search).
Out of interest, is changing the LDAP settings live? (I'd want to be able to fix a "broken" autocomplete without closing the compose window.)

+    // Find out where to insert the card.
+    var insertPosition = 0;
+ 
+    // Next sort on full address
+    while (insertPosition < this._result._searchResults.length &&
+           emailAddress > this._result._searchResults[insertPosition].value)
+      ++insertPosition;
+
+    this._result._searchResults.splice(insertPosition, 0, {
+      value: emailAddress,
+      card: card,
+    });
Autocomplete doesn't expect search results to mutate as autocomplete progresses. Specifically, this code could change the data for a row that the user had already selected.

+    if (topic == "compose-window-close") {
+      // Force the individual query items to null, so that the memory
+      // gets collected straight away.
+      for (var item in this._cachedQueries) {
+        this._cachedQueries[item].query = null;
+        this._cachedQueries[item].book = null;
+      }
+      this._cachedQueries = {};
+      Components.classes["@mozilla.org/observer-service;1"]
+                .getService(Components.interfaces.nsIObserverService)
+                .removeObserver(this, "compose-window-close");
This is the wrong time to remove the observer. Alternatively you could implement nsISupportsWeakReference and add yourself as a weak observer.

+    if (this._result.matchCount) {
+      this._result.searchResult = ACR.RESULT_SUCCESS_ONGOING;
+      this._result.defaultIndex = 0;
This will make autocomplete default to the first found index... if you want.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

While we'd love to get this for 3.0b1, we wouldn't actually hold the release for it, so moving the target milestone out.

Comment 5

[Mark Banner (:standard8)]

This doesn't need to block TB 3 any more, we have a way around it if we need it. Still very much wanted as it would make it easier for extensions.

Resetting assignee to default as I don't have time to work on this at the moment.

Comment 6

[Robert Kaiser]

Any chance to get this updated to trunk? The attached WIP fails to apply and I'd like to see how far we can compile with libxul right now. IMHO, doing that should be a top priority for mozilla-2.0-based releases, but we break in LDAP right now - is this patch alone supposed to fix that or do we need even more?

Comment 7

[Mark Banner (:standard8)]

I'm going to try and get my head back around this patch now that bug 343332 is almost fixed.

Comment 8

[Mark Banner (:standard8)]

Attachment: WIP v2

Updated patch which needs the patch in bug 633150 to be able to apply and work.

This is basically an update to get the original WIP patch working again, and certainly needs work in the new ldap autocomplete component before its finished, as well as some general tidy up I expect. I also suspect there's some more code to be removed from other areas as well.

Comment 9

[Jan Horak [:jhorak]]

I've tried the WIPv2 patch with latest comm-central. I've managed to compile it successfully but autocomplete does not work in compose. Is there any way I can help? This patch is one of few that are missing to link comm-central against libxul.

Comment 10

[[e-mail]]

Comment on attachment 511359 [details] [diff] [review]
WIP v2

+    args.expression = abMgr.convertQueryString(searchStr);
This got renamed to convertQueryStringToExpression but after fixing that I'm still getting an error: Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIAbDirectoryQuery.doQuery]

Comment 11

[[e-mail]]

Comment on attachment 511359 [details] [diff] [review]
WIP v2

>+    var supportsString = Components.classes["@mozilla.org/supports-cstring;1"]
>+      .createInstance(Components.interfaces.nsISupportsCString);
>+
>+    supportsString.data = attributes;
>+
>+    args.typeSpecificArg = supportsString;
This needs to be an nsIAbLDAPAttributeMap. I was able to get search results to show up by using queryObject.book.attributeMap here.

Comment 12

[[e-mail]]

This code snippet seems to create an appropriate map. I don't know whether we should cache the map; it doesn't seem worthwhile creating it each time:

attributes =
    Components.classes["@mozilla.org/addressbook/ldap-attribute-map;1"]
              .createInstance(Components.interfaces.nsIAbLDAPAttributeMap);
attributes.setAttributeList("DisplayName", "cn,commonname", false);
attributes.setAttributeList("PrimaryEmail", "mail", false);

Comment 13

[[e-mail]]

(In reply to comment #11)
> (From update of attachment 511359 [details] [diff] [review])
> >+    var supportsString = Components.classes["@mozilla.org/supports-cstring;1"]
> >+      .createInstance(Components.interfaces.nsISupportsCString);
> >+
> >+    supportsString.data = attributes;
> >+
> >+    args.typeSpecificArg = supportsString;
> This needs to be an nsIAbLDAPAttributeMap. I was able to get search results
> to show up by using queryObject.book.attributeMap here.
Ah, I see now, I failed to apply the patch to nsAbLDAPDirectoryQuery.cpp

Comment 14

[[e-mail]]

Attachment: Proof of concept

This seems to work in SeaMonkey.

This patch does not include any file removals or Thunderbird-specific changes.

Comment 15

[Jan Horak [:jhorak]]

It doesn't seems to work in Thunderbird. Searching in Address book works just fine, but when composing new message an ldap lookup is not performed during typing character in To: field. Only addresses from local address book are shown (I've got Preferences/Composition/Addressing/Address Autocompletion/Directory server enabled).

Comment 16

[[e-mail]]

Attachment: Functional patch

Including the bits of 511359 that I didn't include last time.

I built Thunderbird just to prove that it would work, which it did.

Comment 17

[Jan Horak [:jhorak]]

Attachment: fix of nsAbLDAPAutoCompleteSearch.js

I managed to fix latest patch for me. It missed following code:
  getLabelAt: function getLabelAt(aIndex) {
    return this.getValueAt(aIndex);
  },
Attaching patch as reference. It's working just fine now. What's required to finish this bug?

Comment 18

[Mark Banner (:standard8)]

(In reply to jhorak from comment #17)
> Attaching patch as reference. It's working just fine now. What's required to
> finish this bug?

I believe there are two things:

1) Patch Lightning. This uses the LDAP autocomplete on the event attendees dialog, and we should get it hooked up there.

2) Teach nsAbLDAPAutoCompleteSearch to respect the ldap_2.<server>.autoComplete.filterTemplate preference. This seems to be frequently tweaked and known having done a google search (hence I don't think we can drop it).

This latter issue is probably where I didn't get time to complete the patch. For it, we need to do:

- a bit of work at the end of nsAbLDAPAutoCompleteSearch.startSearch to get the preference if it is set (I wonder if its worth adding a default pref at the same time). This work will also need to make sure all the parameters are in the attribute map that gets passed to the query.

- Update nsAbLDAPAutoCompleteSearch._checkEntry to also support the preference (this may be the more difficult bit, as we either need to hook into the search capabilities of nsAbDirectoryQuery, or re-implement them ourselves. I'd certainly prefer to avoid a re-search of LDAP for a sub-query where we already have query results.


Also, I'm not sure if the current LDAP implementation supports returning results where the display name has matched, and the SecondEmail is listed. If it does, then we should implement that here. If not, then we can always do that later (or just move to supporting mulit-valued mail elements).

Comment 19

[Jan Horak [:jhorak]]

Attachment: up to date patch

Attaching patch which can be applied on current trunk.

Comment 20

[Jan Horak [:jhorak]]

Attachment: up to date patch

Oh, sorry I forgot to hg add some files to patch.

Comment 21

[[e-mail]]

Attachment: Add configure option

This adds a configure option --enable-incomplete-toolkit-ldap-autocomplete and then there are ifdefs in all the core places so that only the selected components actually get built.

I could move the option out of the LDAP block if you like, it just means an extra ifdef in the manifest. (The other uses are already ifdef MOZ_LDAP_XPCOM.)

No ifdefs are required in the compose code as it is written to handle --disable-ldap anyway.

Comment 22

[Jan Horak [:jhorak]]

(In reply to [e-mail] from comment #21)
> Created attachment 620741 [details] [diff] [review]
> Add configure option
> 
> This adds a configure option --enable-incomplete-toolkit-ldap-autocomplete
> and then there are ifdefs in all the core places so that only the selected
> components actually get built.
> 
> I could move the option out of the LDAP block if you like, it just means an
> extra ifdef in the manifest. (The other uses are already ifdef
> MOZ_LDAP_XPCOM.)
> 
> No ifdefs are required in the compose code as it is written to handle
> --disable-ldap anyway.

I'm a bit confused here, I can't see enable-incomplete-toolkit-ldap-autocomplete parameter in the patch. Do I miss something?

Comment 23

[[e-mail]]

Attachment: Really add configure option [Checked in: Comment 26]

D'oh, you'd think that for a patch that adds a configure option that I would remember to include configure.in in my diff...

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 622350 [details] [diff] [review]
Really add configure option
[Checked in: Comment 26]

Review of attachment 622350 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, I'm happy with this. I've not looked at nsAbLDAPAutoCompleteSearch.js as I basically wrote most of that and I think we've done some back and forth before over fixing it that it is good enough for the experimental option.

::: suite/mailnews/compose/addressingWidgetOverlay.xul
@@ +78,5 @@
>          <textbox id="addressCol2#1" class="plain textbox-addressingWidget uri-element"
>                   aria-labelledby="addressCol1#1"
>                   type="autocomplete" flex="1" maxrows="4"
>                   newlines="replacewithcommas"
> +                 autocompletesearch="mydomain addrbook ldap" timeout="300"

This change needs to be done in calendar-event-dialog-attendees.xml as well (this can be included in my r+).

Comment 25

[Mozilla RelEng Bot]

Try run for 2d9fc5ebbe42 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2d9fc5ebbe42
Results (out of 38 total builds):
    exception: 1
    success: 23
    warnings: 11
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2d9fc5ebbe42

Comment 26

[[e-mail]]

Comment on attachment 622350 [details] [diff] [review]
Really add configure option
[Checked in: Comment 26]

Pushed changeset dff73c42414f to comm-central.

Comment 27

[Mozilla RelEng Bot]

Try run for e28d9dcf40e6 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e28d9dcf40e6
Results (out of 37 total builds):
    exception: 1
    success: 19
    warnings: 15
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e28d9dcf40e6

Comment 28

[Jan Horak [:jhorak]]

Attachment: Adding missing function getLabelAt() [Checked in: Comment 31]

This function is missing in nsAbLDAPAutoCompleteSearch.js and it's called during autocompletion of address in compose window.

Comment 29

[Jan Horak [:jhorak]]

Comment on attachment 622350 [details] [diff] [review]
Really add configure option
[Checked in: Comment 26]

I can build TB with external linkages now. This seems to be for me very promising in order to build with external libxul.

Comment 30

[Jan Horak [:jhorak]]

Setting checkin-needed for attachment: https://bug452232.bugzilla.mozilla.org/attachment.cgi?id=624752

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Attachment 624752 [details] [diff] committed to comm-central as https://hg.mozilla.org/comm-central/rev/490b4c844b34

Comment 32

[Jan Horak [:jhorak]]

(In reply to Mark Banner (:standard8) from comment #18)
> 2) Teach nsAbLDAPAutoCompleteSearch to respect the
> ldap_2.<server>.autoComplete.filterTemplate preference. This seems to be
> frequently tweaked and known having done a google search (hence I don't
> think we can drop it).
> 
> This latter issue is probably where I didn't get time to complete the patch.
> For it, we need to do:
> 
> - a bit of work at the end of nsAbLDAPAutoCompleteSearch.startSearch to get
> the preference if it is set (I wonder if its worth adding a default pref at
> the same time). This work will also need to make sure all the parameters are
> in the attribute map that gets passed to the query.
I'm looking at ldap_2.<server>.autoComplete.filterTemplate preference. In code I see that from filter string (like (or(PrimaryEmail,bw,%v)(DisplayName,bw,%v))) is created nsIAbBooleanExpression which is later used to create ldap filter expression: (|(sn=%v*)...). The ldap_2.<server>.autoComplete.filterTemplate variable is expected to be ldap filter expression string.

Current situation:
args.expression = abMgr.convertQueryStringToExpression(searchStr);
in:
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbLDAPAutoCompleteSearch.js#298

conversion to ldap filter expression:
rv = nsAbBoolExprToLDAPFilter::Convert(map, expression, filter);
in:
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp#433

I would rather produce ldap filter expression string in nsAbLDAPAutoCompleteSearch.startSearch (while there is a search term and default filter experssion can be easily overrided it by content of ldap_2.<server>.autoComplete.filterTemplate) and pass it to nsAbLDAPDirectoryQuery::DoQuery. However this would break nsIAbDirectoryQuery interface. Any hints?

Comment 33

[Jan Horak [:jhorak]]

Attachment: Support for ldap_2.<server>.autoComplete.filterTemplate preference v1

Patch is using nsILDAPService::CreateFilter to create filter from filterTemplate and search string.

Comment 34

[Mark Banner (:standard8)]

Comment on attachment 656435 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v1

Somehow, I didn't see this when it came in, and unfortunately this is the same as the previous patch on this bug - attachment 624752 [details] [diff] [review].

Comment 35

[Jan Horak [:jhorak]]

Attachment: Support for ldap_2.<server>.autoComplete.filterTemplate preference v2

Oh dear, I've attached wrong patch, sorry about it. This is the right one.

Comment 36

[Mark Banner (:standard8)]

Comment on attachment 664381 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v2

Review of attachment 664381 [details] [diff] [review]:
-----------------------------------------------------------------

I think you're heading in the right direction, but the problem with doing the work of forming the filter in nsAbLDAPDirectoryQuery is that it will affect all LDAP queries, not just the autocomplete ones.

What I'd suggest you do is to move the filter formation into nsAbLDAPAutoCompleteSearch.startSearch replacing the existing searchStr values (which were just there temporarily anyway). You can then pass that filter via the query args.

You'll want to use the same default as we have now for that string as well (http://hg.mozilla.org/comm-central/annotate/246b6dfaf2e7/mailnews/addrbook/src/nsLDAPAutoCompleteSession.cpp#l39).

Comment 37

[Jan Horak [:jhorak]]

Attachment: Support for ldap_2.<server>.autoComplete.filterTemplate preference v3

Thanks for looking, I hope I got it right. Attaching new patch which moves creating of filter to nsAbLDAPAutoCompleteSearch.js.

Comment 38

[[e-mail]]

Comment on attachment 664906 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v3

>+    var ldapSvc = Components.classes["@mozilla.org/network/ldap-service;1"]
>+                            .createInstance(Components.interfaces.nsILDAPService);
[Not read the patch thoroughly but this should probably use getService]

Comment 39

[Mark Banner (:standard8)]

Comment on attachment 664906 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v3

A quick glance and this is what I was thinking of, so I'll try and take a look at it tomorrow.

Comment 40

[[e-mail]]

Comment on attachment 664906 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v3

>+    var filterTemplate;
>+    // Check if autoComplete.filterTemplate pref for selected ldap server exists
>+    try {
>+      var filterTemplate = prefSvc.getCharPref(acDirURI + ".autoComplete.filterTemplate");
>+    } catch (e) { }
>+    
>+    // When filter template not found use default template
>+    if (!filterTemplate)
>+      filterTemplate = "(|(cn=%v1*%v2-*)(mail=%v1*%v2-*)(sn=%v1*%v2-*))";
It might be possible to replace this with
filterTemplate = book.getStringValue("autoComplete.filterTemplate",
                                     "(|(cn=%v1*%v2-*)(mail=%v1*%v2-*)(sn=%v1*%v2-*))");

Comment 41

[Jan Horak [:jhorak]]

Attachment: Support for ldap_2.<server>.autoComplete.filterTemplate preference v4

Thanks for comments, attaching better version (getService and usage of queryObject.book).

Comment 42

[Jan Horak [:jhorak]]

I'm looking at this one now:

(In reply to Mark Banner (:standard8) from comment #18)
> - Update nsAbLDAPAutoCompleteSearch._checkEntry to also support the
> preference (this may be the more difficult bit, as we either need to hook
> into the search capabilities of nsAbDirectoryQuery, or re-implement them
> ourselves. I'd certainly prefer to avoid a re-search of LDAP for a sub-query
> where we already have query results.

I'm not sure how to solve it. It might be possible to map filterTemplate content to list of card parameters (something like opposite of nsAbBoolExprToLDAPFilter::Convert) and then use _checkEntry to test if search string matches. This could be more complicated for ldap binary operators. 

Could you please elaborate hooking into nsAbDirectryQuery a little further? Do you mean to somehow modify DoQuery to use cached search results?

Comment 43

[[e-mail]]

Doesn't the XPFE code re-query each time? (This has the side-effect of working around the 100-card query result limit.) In which case, if you did want to add caching, perhaps you could just limit it to the common case of no template.

Comment 44

[Mark Banner (:standard8)]

(In reply to [e-mail] from comment #43)
> Doesn't the XPFE code re-query each time? (This has the side-effect of
> working around the 100-card query result limit.) In which case, if you did
> want to add caching, perhaps you could just limit it to the common case of
> no template.

Not sure about XPFE version of the LDAP backend, but the caching already works in the auto complete module we've implemented so far (with xpfe autocomplete).

Comment 45

[Mark Banner (:standard8)]

Comment on attachment 665324 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v4

Review of attachment 665324 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/public/nsIAbDirectoryQuery.idl
@@ +39,5 @@
>       * A parameter which can be used to pass in data specific to a particular
>       * type of addressbook.  
>       */
>      attribute nsISupports typeSpecificArg;
> +    

nit: drop the spaces from this line and the one just after the function definition.

::: mailnews/addrbook/src/nsAbLDAPAutoCompleteSearch.js
@@ +290,5 @@
>        Components.classes["@mozilla.org/addressbook/directory/query-arguments;1"]
>                  .createInstance(Components.interfaces.nsIAbDirectoryQueryArguments);
>  
> +    var filterTemplate = queryObject.book.getStringValue("autoComplete.filterTemplate",
> +                         "(|(cn=%v1*%v2-*)(mail=%v1*%v2-*)(sn=%v1*%v2-*))");    

nit: whitespace at end of line.

::: mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp
@@ +428,5 @@
> +
> +  if (filter.IsEmpty()) {
> +    // Get the filter
> +    nsCOMPtr<nsISupports> supportsExpression;
> +    rv = aArguments->GetExpression (getter_AddRefs (supportsExpression));

nit: no space before open brackets (and for the expression line), these were there before, but please tidy as you're touching them.

@@ +435,5 @@
> +    nsCOMPtr<nsIAbBooleanExpression> expression (do_QueryInterface (supportsExpression, &rv));
> +
> +    // figure out how we map attribute names to addressbook fields for this
> +    // query
> +    rv = nsAbBoolExprToLDAPFilter::Convert(map, expression, filter);

Shouldn't we be checking rv here?

Comment 46

[Mark Banner (:standard8)]

Comment on attachment 665324 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v4

This is looking good, but I'd like to see an updated version with comment 45 items fixed.

Comment 47

[Jan Horak [:jhorak]]

Attachment: Support for ldap_2.<server>.autoComplete.filterTemplate preference v5

Fixed nits and throwing exception when filter is empty.

Comment 48

[Mark Banner (:standard8)]

Comment on attachment 669912 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v5

I think you got the title wrong for this patch, so updating it.

Comment 49

[Mark Banner (:standard8)]

Comment on attachment 669912 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v5

Review of attachment 669912 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. This looks fine.

Comment 50

[Mark Banner (:standard8)]

(In reply to jhorak from comment #42)
> I'm looking at this one now:
> 
> (In reply to Mark Banner (:standard8) from comment #18)
> > - Update nsAbLDAPAutoCompleteSearch._checkEntry to also support the
> > preference (this may be the more difficult bit, as we either need to hook
> > into the search capabilities of nsAbDirectoryQuery, or re-implement them
> > ourselves. I'd certainly prefer to avoid a re-search of LDAP for a sub-query
> > where we already have query results.
> 
> I'm not sure how to solve it. It might be possible to map filterTemplate
> content to list of card parameters (something like opposite of
> nsAbBoolExprToLDAPFilter::Convert) and then use _checkEntry to test if
> search string matches. This could be more complicated for ldap binary
> operators. 
> 
> Could you please elaborate hooking into nsAbDirectryQuery a little further?
> Do you mean to somehow modify DoQuery to use cached search results?

I think I was probably wondering if we could hook into nsAbDirectoryQuery::matchCard (or split that out to a separate module) and pass a boolean expression converted from the filter, and use those match routines rather than the ones in _checkEntry.

Comment 51

[Jan Horak [:jhorak]]

Is s-r required as long as nsIAbDirectoryQuery.idl was changed?

Comment 52

[Mark Banner (:standard8)]

Comment on attachment 669912 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v5

Oh yes, forgot about that, I'm sure Neil will take a quick look.

Comment 53

[[e-mail]]

Comment on attachment 669912 [details] [diff] [review]
Support for ldap_2.<server>.autoComplete.filterTemplate preference v5

>+    var filterTemplate = queryObject.book.getStringValue("autoComplete.filterTemplate",
>+                         "(|(cn=%v1*%v2-*)(mail=%v1*%v2-*)(sn=%v1*%v2-*))");
 
>+    // If autoComplete.filterTemplate is set to empty string use default value
>+    if (!filterTemplate)
>+      filterTemplate = "(|(cn=%v1*%v2-*)(mail=%v1*%v2-*)(sn=%v1*%v2-*))";
I'm not too keen on the duplication of the default string. If you want to guard against someone accidentally setting a zero-length preference then I would pass an empty default value, which is then handled by the subsequent check.

>+    // Create filter from filter template and search string
>+    var ldapSvc = Components.classes["@mozilla.org/network/ldap-service;1"]
>+                            .getService(Components.interfaces.nsILDAPService);
>+    var filter = ldapSvc.createFilter(1024, filterTemplate, "", "", "", aSearchString);
>+    if (!filter)
>+      throw new Error("Filter string is empty, check if filterTemplate variable is valid in prefs.js.");
>     args.typeSpecificArg = queryObject.attributes;
>     args.querySubDirectories = true;
>+    args.filter = filter;
createFilter's return value is documented to be an AUTF8String, so the new filter attribute should be the same otherwise XPConnect will trash the high bytes. sr=me with that fixed.

Comment 54

[Jan Horak [:jhorak]]

Attachment: Save uri for downloaded files patch v6 [Checked in: Comment 56]

Attaching fixed patch, copying positive reviews.

Comment 55

[Jan Horak [:jhorak]]

(In reply to Mark Banner (:standard8) from comment #50)
> I think I was probably wondering if we could hook into
> nsAbDirectoryQuery::matchCard (or split that out to a separate module) and
> pass a boolean expression converted from the filter, and use those match
> routines rather than the ones in _checkEntry.

So we need code which will convert the filterTemplate to the boolean expression, right?

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/4450eb62b1e7

Comment 57

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, this caused build bustage. Backed out.
https://hg.mozilla.org/comm-central/rev/36b49398e65c

https://tbpl.mozilla.org/php/getParsedLog.php?id=16905255&tree=Thunderbird-Trunk

Comment 58

[[e-mail]]

Comment on attachment 678693 [details] [diff] [review]
Save uri for downloaded files patch v6 [Checked in: Comment 56]

>+  nsCAutoString filter;
...
>-  nsAutoCString filter;
I didn't spot it before, but this change looks erroneous.

Comment 59

[Philip Chee]

> I didn't spot it before, but this change looks erroneous.
But of course:

Bug 773151 - Rename nsCAutoString to nsAutoCString

Comment 60

[[e-mail]]

The patch compiles for me with that fixed.

Comment 61

[[e-mail]]

Comment on attachment 678693 [details] [diff] [review]
Save uri for downloaded files patch v6 [Checked in: Comment 56]

Pushed comm-central changeset 8c52fbaa4410 with the nsAutoCString change.

Comment 62

[Takanori MATSUURA]

(In reply to [e-mail] from comment #61)
> Pushed comm-central changeset 8c52fbaa4410 with the nsAutoCString change.

Leave open or fixed?

Comment 63

[Mark Banner (:standard8)]

(In reply to Takanori MATSUURA from comment #62)
> (In reply to [e-mail] from comment #61)
> > Pushed comm-central changeset 8c52fbaa4410 with the nsAutoCString change.
> 
> Leave open or fixed?

Leave open. To be explicit there are two items remaining (which I know jhorak knows about, but commenting here to be explicit):

- The last couple of parts in comment 18, namely:

> - Update nsAbLDAPAutoCompleteSearch._checkEntry to also support the
> preference (this may be the more difficult bit, as we either need to hook
> into the search capabilities of nsAbDirectoryQuery, or re-implement them
> ourselves. I'd certainly prefer to avoid a re-search of LDAP for a sub-query
> where we already have query results.
> 
> 
> Also, I'm not sure if the current LDAP implementation supports returning
> results where the display name has matched, and the SecondEmail is listed.
> If it does, then we should implement that here. If not, then we can always
> do that later (or just move to supporting mulit-valued mail elements).

- Actually enabling the toolkit interfaces by default and full removal of the code (a switch to use toolkit autocomplete itself should be done probably in bug 360648 and/or others).

Comment 64

[Jan Horak [:jhorak]]

While working on patch, I've realized that ldap query doesn't return all records matching typed search term (this is probably set on Preferences/Composition/LDAP directory servers/Edit/Advanced/Don't return more than X results - by default 30). This of course breaks reusing already obtained LDAP results (because results are incomplete). 

I've got working prototype but it's not useful, when there are missing records which wasn't found by last ldap search with limit 30.
Any hint or workaround? Maybe start to using it after we have less than maximum ldap results returned? And is it worth?

Comment 65

[Jan Horak [:jhorak]]

Current ldap autocomplete implementation in JS is broken. For example if I'm looking for "Andrew Someone" and type "Andrew " there's only one record left. When I press backspace more Andrews appear in drop down menu. 

This is because we're trying to filter previous result of ldap query (here: http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbLDAPAutoCompleteSearch.js#188
Because this query doesn't have to contain all records stored on ldap server (there's query limit 30 I believe - maxHits) in the end we end up with missing records.

I found this issue only when building with --enable-incomplete-toolkit-ldap-autocomplete.

My LDAP server for some reason doesn't return maxHits records even when there should be is more than maxHits records returned so I can't use count of returned records to determine whenever server returned all remaining records (ie. I can use _checkEntry) or there might be more results (count == maxHits, ie. have to redo ldap query).

Comment 66

[Mark Banner (:standard8)]

(In reply to jhorak from comment #64)
> While working on patch, I've realized that ldap query doesn't return all
> records matching typed search term (this is probably set on
> Preferences/Composition/LDAP directory servers/Edit/Advanced/Don't return
> more than X results - by default 30). This of course breaks reusing already
> obtained LDAP results (because results are incomplete). 
> 
> I've got working prototype but it's not useful, when there are missing
> records which wasn't found by last ldap search with limit 30.
> Any hint or workaround? Maybe start to using it after we have less than
> maximum ldap results returned? And is it worth?

Would it perhaps be better not to cache the ldap results? I'm not actually sure how much of this is going on server side versus client side.

Comment 67

[Jan Horak [:jhorak]]

(In reply to Mark Banner (:standard8) from comment #66)
> > I've got working prototype but it's not useful, when there are missing
> > records which wasn't found by last ldap search with limit 30.
> > Any hint or workaround? Maybe start to using it after we have less than
> > maximum ldap results returned? And is it worth?
> 
> Would it perhaps be better not to cache the ldap results? I'm not actually
> sure how much of this is going on server side versus client side.

I guess so, there's no caching in current implementation (without --enable-incomplete-toolkit-ldap-autocomplete) AFAIK.

Comment 68

[Mark Banner (:standard8)]

Attachment: Remove cache option [checked in: comment 80]

Neil and I discussed this on irc the other day. Given the cache issues, and the fact that we didn't have a cache before, we agreed that we just shouldn't use the cache option.

Therefore this patch removes the partially implemented cache option. I'll add a patch in a bit for enabling the toolkit based search by default.

Comment 69

[[e-mail]]

Comment on attachment 772362 [details] [diff] [review]
Remove cache option [checked in: comment 80]

>-
>     // Here we need to do the real search, as we haven't got any previous
>     // results to fall back on.
> 
Nit: This comment is no longer relevant. (Although now I begin to wonder whether we need to cache the identity...)

This looks reasonable but jcranmer has broken LDAP on all of my external API builds so I can't test it right now.

Comment 70

[Mark Banner (:standard8)]

Attachment: Handle undefined parameter - e.g. mailing lists

For mailing lists, we don't have an identity, therefore we don't have the parameter. So for now, we should just handle it and seek out the default identity.

Comment 71

[Mark Banner (:standard8)]

Attachment: Remove the build option, switch to toolkit interfaces (build config parts)

This patch removes the build option, and switch to toolkit autocomplete interfaces by default. Requesting Joshua as build peer review, and Neil for sr.

Comment 72

[Mark Banner (:standard8)]

Attachment: Switch to toolkit interfaces, remove old js code

This removes all the old ldap set-up code (yay!), and with the previous patches, completes the switch.

Comment 73

[Joshua Cranmer [:jcranmer]]

If I'm not mistaken, these changes mean we would no longer need to build mozilla/xpfe/components/autocomplete?

Comment 74

[Mark Banner (:standard8)]

(In reply to Joshua Cranmer [:jcranmer] from comment #73)
> If I'm not mistaken, these changes mean we would no longer need to build
> mozilla/xpfe/components/autocomplete?

From what Neil has said before to me, we would still need some of the content based code until we completely switch to toolkit interfaces. We would be able to get rid of the binary parts that we have now (with some modification of the toolkit parts).

Comment 75

[Joshua Cranmer [:jcranmer]]

Comment on attachment 772402 [details] [diff] [review]
Remove the build option, switch to toolkit interfaces (build config parts)

Review of attachment 772402 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/src/moz.build
@@ +73,5 @@
>  EXTRA_COMPONENTS += [
>      'nsAbAutoCompleteMyDomain.js',
>      'nsAbAutoCompleteSearch.js',
>      'nsAbLDAPAttributeMap.js',
> +    'nsAbLDAPAutoCompleteSearch.js',

This change isn't quite right, it adds the component even when LDAP is disabled.

::: mailnews/addrbook/src/nsAddrbook.manifest
@@ -6,4 @@
>  contract @mozilla.org/addressbook/ldap-attribute-map;1 {127b341a-bdda-4270-85e1-edff569a9b85}
>  component {4ed7d5e1-8800-40da-9e78-c4f509d7ac5e} nsAbLDAPAttributeMap.js
>  contract @mozilla.org/addressbook/ldap-attribute-map-service;1 {4ed7d5e1-8800-40da-9e78-c4f509d7ac5e}
> -#ifdef MOZ_INCOMPLETE_TOOLKIT_LDAP_AUTOCOMPLETE

Similarly, instead of being deleted, this should just change to MOZ_LDAP_XPCOM.

Comment 76

[[e-mail]]

(In reply to Mark Banner from comment #74)
> (In reply to Joshua Cranmer from comment #73)
> > If I'm not mistaken, these changes mean we would no longer need to build
> > mozilla/xpfe/components/autocomplete?
> 
> From what Neil has said before to me, we would still need some of the
> content based code until we completely switch to toolkit interfaces. We
> would be able to get rid of the binary parts that we have now (with some
> modification of the toolkit parts).

To clarify, mozilla/xpfe/components/autocomplete has three parts:
1. src - only needed by LDAP C++ autocomplete and its consumers
2. public - needed by src; currently needed by resources/content
3. resources/content - needed by SeaMonkey; currently needed by Thunderbird

So once the LDAP C++ autocomplete is removed, the XPFE C++ can be removed, then I can write a patch to remove the legacy interface support, but meanwhile Thunderbird can also investigate switching to toolkit autocomplete XBL at which point resources/content can be moved to suite/common/bindings.

Comment 77

[[e-mail]]

Comment on attachment 772400 [details] [diff] [review]
Handle undefined parameter - e.g. mailing lists

>+    if (!aParam) {
>+      try {
>+        this._cachedIdentity =
>+          MailServices.accounts.defaultAccount.defaultIdentity;
>+      }
>+      catch (ex) {
>+        Components.utils.reportError("LDAP autocomplete could not get" +
>+                                     "default identity");
I didn't even realise that Thunderbird had LDAP autocomplete on its mailing lists, so I looked at the code, and it currently seems to use the global LDAP preferences, which would make slightly more sense to me. Note that it's not strictly necessary to catch the exception as the autocomplete widget will ignore the search if startSearch throws.

>+        this._result.searchResult = ACR.RESULT_IGNORED;
>+        aListener.onSearchResult(this, this._result);
>+        return;        
>+      }
>+    }
>+    else if (aParam != this._cachedParam) {
>       this._cachedIdentity = MailServices.accounts.getIdentity(aParam);
>       this._cachedParam = aParam;
>     }
This breaks the cache because if you compose using a particular identity, then edit an address list, then compose using that identity again, you end up with the default identity instead. (Maybe just get rid of the cache?)

Comment 78

[[e-mail]]

Comment on attachment 772403 [details] [diff] [review]
Switch to toolkit interfaces, remove old js code

-                   autocompletesearch="addrbook" timeout="300" maxrows="4" 
+                   autocompletesearch="addrbook ldap" timeout="300" maxrows="4" 
[I guess I need to add this to suite/mailnews/addrbook/abListOverlay.xul too]

>-
>+
Go home Mercurial, you're drunk.

Comment 79

[[e-mail]]

Comment on attachment 772362 [details] [diff] [review]
Remove cache option [checked in: comment 80]

Oh yes, this is a world of improvement.

r=me with that comment fix as previously noted.

Comment 80

[Mark Banner (:standard8)]

Comment on attachment 772362 [details] [diff] [review]
Remove cache option [checked in: comment 80]

https://hg.mozilla.org/comm-central/rev/1474fbd5ab3d

Comment 81

[Joshua Cranmer [:jcranmer]]

Comment on attachment 772403 [details] [diff] [review]
Switch to toolkit interfaces, remove old js code

Review of attachment 772403 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not going to purport to be an expert on this stuff, but looks good to me. Sorry for the delay, I got sidetracked when reviewing this.

::: mail/components/addrbook/content/abMailListDialog.xul
@@ +67,4 @@
>          <listcell class="addressingWidgetCell">
>            <textbox id="addressCol1#1" class="plain textbox-addressingWidget uri-element"
>                     type="autocomplete" flex="1"
> +                   autocompletesearch="addrbook ldap" timeout="300" maxrows="4" 

Remove trailing whitespace while you're here?

Comment 82

[Mark Banner (:standard8)]

Attachment: Switch to toolkit interfaces, remove old js code v2

With comments addressed

Comment 83

[Mark Banner (:standard8)]

Attachment: Remove the build option, switch to toolkit interfaces (build config parts) v2

With comments addressed

Comment 84

[Mark Banner (:standard8)]

Attachment: Handle undefined parameter - e.g. mailing lists v2 [checked in comment 88]

Addressed review comments - removed the identity cache and use the global preference if appropriate.

Comment 85

[Mark Banner (:standard8)]

Attachment: Switch to toolkit interfaces, remove old js code v3 [checked in comment 88]

Correct patch.

Comment 86

[Mark Banner (:standard8)]

Attachment: Remove the build option, switch to toolkit interfaces (build config parts) v3 [checked in comment 88]

Correct version

Comment 87

[[e-mail]]

Comment on attachment 777045 [details] [diff] [review]
Handle undefined parameter - e.g. mailing lists v2 [checked in comment 88]

Nice!

Comment 88

[Mark Banner (:standard8)]

Landed the last three parts.

https://hg.mozilla.org/comm-central/rev/f0535a4d8727
https://hg.mozilla.org/comm-central/rev/c06df11f1abf
https://hg.mozilla.org/comm-central/rev/0ba38b41f77f

If there are any issues following this, please file them as new bugs.

Comment 89

[Serge Gautherie (:sgautherie)]

(PS: Should you remove "[leave open]" on this bug whiteboard ?)

Comment 90

[Jens Müller (:tessarakt)]

In mail/components/compose/content/addressingWidgetOverlay.js there is still this code:

      //this copies the autocomplete sessions list from recipient#1
      input[0].syncSessions(document.getElementById('addressCol2#1'));

Is this still required?

Comment 91

[[e-mail]]

(In reply to Jens Müller from comment #90)
> In mail/components/compose/content/addressingWidgetOverlay.js there is still
> this code:
> 
>       //this copies the autocomplete sessions list from recipient#1
>       input[0].syncSessions(document.getElementById('addressCol2#1'));
> 
> Is this still required?

No, see the dependency on bug 899822 that I recently added.