1670752 - [API Request] Contacts search provider API

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, enhancement)

Description

[Ben Bucksch (:BenB)]

This is a MailExtension API which allows a MailExtension to serve as address book provider and contribute autocomplete results when the user types a name in the composer To: field, and other similar places.

The search starts only after the user typed, and can run the search on a server, and return only the matching results. This is useful for global company user databases that often have 30 000 or even 200 000 entries and cannot be synced continuously. (Even if the network traffic was OK, TB AB locks up with that many entries.)

This allows extensions to open up a great variety of contacts sources, from custom protocols (ProtonMail etc.), over web forums, to - most importantly - connection to company customer databases like Open-Source CRMs, SAP or whatever is used in a company.

Details to follow

Comment 1

[Ben Bucksch (:BenB)]

Attachment: Initial version

First draft, not yet ready for review

Comment 2

[Ben Bucksch (:BenB)]

Code

The extension code then looks something like this:

messenger.addressBooks.onAutoComplete.addListener(searchString => {
  return [{
    DisplayName: "First " + searchString,
    PrimaryEmail: "first." + searchString + "@beonex.com",
  },{
    DisplayName: "Second " + searchString,
    PrimaryEmail: "second." + searchString + "@beonex.com",
  }];
});

The search results would of course typically come from a server.

The listener is the only function you have to implement. When adding the listener, you can specify a number of optional properties, such as the address book's name.

UI

• The address book appear in the address book UI window
  • If the user clicks on it, no cards appear (if you implemented only this function)
  • If the user types something in the search bar, search results appear as AB cards, and user can open each card
• When composing and email and typing into recipients To/Cc fields, the autocomplete triggers the listener above and starts a search, and then shows the search results in the autocomplete dropdown.

Comment 3

[[e-mail]]

Attachment: Updated to latest TB78 ESR

Comment 4

[Ben Bucksch (:BenB)]

FWIW, this code would be simpler, if the address book manager did not hardcode address book types. I suggest to split the manager into 2 parts:

1. A generic list of address books (editable list, with observers)
2. another class is responsible for the built-in address books and adds them to the list.

But that's outside the scope of this bug.

Comment 5

[Mark Banner (:standard8)]

I have some concerns about the approaches here. These stem from the fact that I don't think the API hasn't been proposed/discussed on topicbox - which I believe is the route we're generally heading for (I'd generally expect that discussion before review, which is why I'm raising it here).

It feels like the wrong design - it bills it self as an autocomplete provider API for contacts, but then it creates a read-only address book. They're two very different things e.g. mydomain vs addrbook.

Additionally if this is intended as an actual contacts API, there's no discussion about how the API would be extended to allow it to become a read-write address book in future, and I don't see an obvious way to do that with this API.

We either need to limit this patch to just autocomplete, in which case I'd probably expect something a little more like the omnibox API that behaves a bit like mydomain does in the backend or we need to consider what a full address book provider API would look like, even if we only implement a read-only part of it for now.

Comment 6

[Ben Bucksch (:BenB)]

it bills it self as an autocomplete provider API for contacts, but then it creates a read-only address book

What's wrong about that? That's what I expect as a user. If I can see autocomplete results, I will want to know where they come from. And I will expect the address book window to have a super-set of all addresses, and to be able to find everything there. It would be mighty strange, if I could type an address in the composer and composer finds the address, but the address book doesn't. We're doing what an end user would expect, because this is a high level API.

LDAP works the same way. It has an entry in the address book window, and it autocompletes. The goal of this API is to allow MailExtensions to implement something functionally equivalent to LDAP, e.g. a connection to a CRM system.

we need to consider what a full address book provider API would look like

There is already a full address book provider MailExtension API, since Thunderbird 68.

For a more general approach, see my comment 4 above.

Comment 7

[Ben Bucksch (:BenB)]

The API here implements a search-only address book.

I.e. TB doesn't know all contacts, but they are returned after searching. This API is useful for all contacts sources where the data is held in a third party system, and with many contacts (potentially 10000 or more).

We've deliberately tried to make the API very simple to implement and therefore high level. It's just one single function. In other words, even if there might be other more low level APIs for specific situations, we would still want to offer this simple one-function API, to make it simple to implement MailExtensions for all those CRMs and similar software out there.

Comment 8

[Mark Banner (:standard8)]

(In reply to Ben Bucksch (:BenB) from comment #6)

it bills it self as an autocomplete provider API for contacts, but then it creates a read-only address book

What's wrong about that? That's what I expect as a user. If I can see autocomplete results, I will want to know where they come from. And I will expect the address book window to have a super-set of all addresses, and to be able to find everything there. It would be mighty strange, if I could type an address in the composer and composer finds the address, but the address book doesn't. We're doing what an end user would expect, because this is a high level API.

It is wrong because it the name is implicating that it is only used for autocomplete, not for display in the address book window as well. Display in the address book window is not expected from something called "autocomplete".

LDAP works the same way. It has an entry in the address book window, and it autocompletes. The goal of this API is to allow MailExtensions to implement something functionally equivalent to LDAP, e.g. a connection to a CRM system.

I think we can do that with a full address book provider, and save the additional, potentially confusing interface.

we need to consider what a full address book provider API would look like

There is already a full address book provider MailExtension API, since Thunderbird 68.

This isn't documented on https://thunderbird-webextensions.readthedocs.io/ and John doesn't know about it either. Do you mean an experiment?

(In reply to Ben Bucksch (:BenB) from comment #7)

The API here implements a search-only address book.

Ah, that's a slightly clearer name.

I.e. TB doesn't know all contacts, but they are returned after searching. This API is useful for all contacts sources where the data is held in a third party system, and with many contacts (potentially 10000 or more).

We've deliberately tried to make the API very simple to implement and therefore high level. It's just one single function. In other words, even if there might be other more low level APIs for specific situations, we would still want to offer this simple one-function API, to make it simple to implement MailExtensions for all those CRMs and similar software out there.

I think we should still consider what a full provider API would need to look like.

There's other concerns I have as well - like card identifiers, other add-ons knowing that this is a search-only book etc. Also, how are remote async searches handled which would probably be needed for this type of book?

Whilst I appreciate this works for your particular case, I'm not convinced it is the best solution.

Comment 9

[Ben Bucksch (:BenB)]

the name is implicating that it is only used for autocomplete, not for display in the address book window as well. Display in the address book window is not expected from something called "autocomplete".

Ah, yes, you're right. The name could be confusing. We will change it to something that better reflects the purpose of the API.

There is already a full address book provider MailExtension API, since Thunderbird 68.

This isn't documented on https://thunderbird-webextensions.readthedocs.io/

If you want to create a local address book, you can do this:

let addressBook = await browser.addressBooks.create({ name });
let contact = await browser.contacts.create(addressBook.id, null, properties);
await browser.contacts.update(contact.id, changes);

https://thunderbird-webextensions.readthedocs.io/en/78/addressBooks.html
https://thunderbird-webextensions.readthedocs.io/en/78/contacts.html

That creates a local address book, using the TB implementation of an address book. If you call search() on it, it will invoke the Thunderbird implementation of the search function, which will search within the known contacts that you pushed in before.

That said, you are right, that's not a "provider" API. I think you are saying that you want extensions to be able to implement the entire address book API themselves. However, from what I can see, I don't have a use case for an extension to implement a full provider. If you need to store the contacts locally, you would need to implement the entire storage and search function locally. It would make more sense to create a TB-implemented local address and then pushing your data into it, using the functions above.

In short, this API solves a different case that you have in mind. This API simply allows Thunderbird to use data that exists in an external, third party data store. The API for the extension should be really simple to implement. We currently have only 1 function. So, you can implement this in a matter of 2 hours, from scratch. That's the goal here.

If we later add a more powerful API with editable entries, we would still support this API for simpler extensions that don't need this power. Having one API function is reasonable for this very common case.

Comment 10

[Mark Banner (:standard8)]

(In reply to Ben Bucksch (:BenB) from comment #9)

There is already a full address book provider MailExtension API, since Thunderbird 68.

let addressBook = browser.addressBooks.create({ name });
let contact = browser.contacts.create(addressBook.id, null, properties);
browser.contacts.update(contact.id, changes);

https://thunderbird-webextensions.readthedocs.io/en/78/addressBooks.html
https://thunderbird-webextensions.readthedocs.io/en/78/contacts.html

I'm not convinced that's a provider. That's a way of creating a local address book and adding items to it - you're not in the same type of control of it, and it requires a local cache. I'd expect something more like the proposed calendar provider API (https://docs.google.com/document/d/15awbKiVfdOTmsRpgD1dxm3gvOt08EQZDSnMl8QRBFoY/edit#heading=h.pk9z1j6pckt3)

This is where having a discussion on topicbox would also help get other developer's perspective.

Comment 11

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #10)

This is where having a discussion on topicbox would also help get other developer's perspective.

(oh you already have started one)

Comment 12

[Ben Bucksch (:BenB)]

it requires a local cache

Right, see my edited comment 9.

If you have suggestions for a better API, and the API is just as simple as our API, for the simple search-only LDAP-like use case, I'd be happy to see your proposals for comparison.

Comment 13

[Geoff Lankow (:darktrojan)]

Comment on attachment 9195597 [details] [diff] [review]
Updated to latest TB78 ESR

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

This patch is well behind trunk so I'm not even considering marking it r+. Also, there's no testing.

Regardless of the ongoing discussion about how it fits in the API, there's a bunch of things here that will need to happen anyway, so I'm going through and pointing out changes that will need to be made.

I'm not actually sure that you need to create a separate autocomplete provider. As long as you're adding an nsIAbDirectory with useForAutocomplete returning true, I think AbAutoCompleteSearch should be able to handle it. I could be wrong though.

::: mail/components/extensions/parent/ext-autoComplete.js
@@ +22,5 @@
> +  ];
> +
> +  let card = Cc[
> +    "@mozilla.org/addressbook/cardproperty;1"
> +  ].createInstance(Ci.nsIAbCard);

You need to set the directoryUID property on card.

@@ +43,5 @@
> +
> +class JsAutoCompleteBook {
> +  constructor(fire, context, args = {}) {
> +    this.fire = fire;
> +    this.uuid = String(args.uuid ?? "");

uuid's not a thing any more.

@@ +48,5 @@
> +    this.readOnly = true;
> +    this.isRemote = true;
> +    this.isSecure = Boolean(args.isSecure);
> +    this.propertiesChromeURI = "chrome://messenger/content/addressbook/abAddressBookNameDialog.xhtml";
> +    this.dirName = String(args.dirName ?? context.extension.name);

The dirName can be changed through the UI but it's not stored anywhere so it resets as soon as Thunderbird closes.

@@ +51,5 @@
> +    this.propertiesChromeURI = "chrome://messenger/content/addressbook/abAddressBookNameDialog.xhtml";
> +    this.dirName = String(args.dirName ?? context.extension.name);
> +    this.dirType = kJsAutoCompleteDirectory;
> +    this.fileName = "";
> +    this.UID = String(args.UID ?? Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator).generateUUID().toString().slice(1, -1));

I don't think there's much point in letting the extension specify the UID. And you can use AddrBookUtils.newUID to make this tidier.

@@ +57,5 @@
> +    this.position = 0;
> +    this.lastModifiedDate = 0;
> +    this.isMailList = false;
> +    this.childNodes = emptyEnumerator;
> +    this.childCards = emptyEnumerator;

Could use AddrBookUtils.SimpleEnumerator([]) and avoid another implementation.

::: mailnews/addrbook/modules/AddrBookManager.jsm
@@ +40,5 @@
>  
>  /** Directory type constants, as defined in nsDirPrefs.h. */
>  const LDAP_DIRECTORY_TYPE = 0;
>  const MAPI_DIRECTORY_TYPE = 3;
> +const JS_AUTOCOMPLETE_TYPE = 100;

These live in nsIAbManager.idl now. Please use 103 rather than 100.

@@ +448,5 @@
> +      );
> +    }
> +
> +    store.set(dir.URI, dir);
> +    this.notifyDirectoryItemAdded(null, dir);

notifyDirectoryItemAdded no longer exists. You should however call updateSortedDirectoryList() here. Add your new directory type to abSortOrder in AddrBookUtils.jsm with the same value as LDAP.

::: mailnews/addrbook/public/nsIAbDirectory.idl
@@ +206,5 @@
>     * VALUEn  The value to be matched in the FIELDn via the OPn operator.
>     *         The value must be URL encoded by the caller, if it contains any
>     *         special characters including '(' and ')'.
>     */
> +  void search(in AString query, in AString searchString, in nsIAbDirSearchListener listener);

Would this new argument be better as an array of string tokens, as split up by getSearchTokens (which handles quoted strings and commas etc.)? Obviously this propagates through to how your API event works, but I think the first thing a good extension would do is perform the same tokenisation. If we do it here then the behaviour is consistent everywhere because it has to be.

(Same question arises from bug 1658131, if we ever get back to it.)

Comment 14

[Geoff Lankow (:darktrojan)]

I think this event should be a member of the addressBooks namespace. Calling it autoComplete is a bit too generic, and there's unlikely to be much else that could go in such a namespace.

Also, we should consider what happens if for some reason the user needs two autocomplete providers from the same extension. An extension could use one listener with data from two sources which would be fine, or it could add a second listener but would have no way of knowing which was being called (e.g. if the user is searching in one directory in the UI). We could pass an identifier to the event (as we do with the cloudFile API), which would also help future-proof it for expansion to a full directory API.

Comment 15

[Ben Bucksch (:BenB)]

this event should be a member of the addressBooks namespace. Calling it autoComplete is a bit too generic

Yes. The name autoComplete was a complete mis-nomer and caused a lot of confusion. We will change that.

This is actually a full address book, also appearing in the address book window as address book. Same as LDAP.

if for some reason the user needs two autocomplete providers from the same extension

That already works. You can attach 2 listeners, with different names, from the same extension. They appear as 2 separate address books, with different results. Our addon does this, and I just verified by testing that this works fine.

Comment 16

[Ben Bucksch (:BenB)]

array of string tokens, as split up by getSearchTokens (which handles quoted strings and commas etc.)? Obviously this propagates through to how your API event works

You mean to split what the user entered into words? We're actually just passing the string to the server verbatim. The server then parses the search string. The reason why we added the new parameter is because we wanted the original string, as the user entered it.

Comment 17

[Ben Bucksch (:BenB)]

Attachment: Screenshot of 2 search-only address books in the address book window

These are full address books, and show up in the address book window, the same as LDAP servers. They also autocomplete in the composer and in the calendar event invitee list.

This shows a sample setup with 2 different such address books, from the same extension, quering 2 different servers, and giving different results for the search search term.

The code is trivial, as shown in comment 2.

Comment 18

[Geoff Lankow (:darktrojan)]

I'm aware, but what I'm getting at is, if we somehow extended this to become more like a full address book provider (which I know isn't the aim here, I'm just thinking ahead) then the 1:1 relationship between directories and events they could possibly fire breaks down. Regardless of how the app side of things works, you'd need some way to differentiate which source you should be providing information from.

For now I'm just putting stuff out there. This bug ties in with a number of other things I have on the back burner that I can't get back to quickly.

You're probably right about the search string. Providers can tokenise it themselves if they want to. It would be nice to have total consistency but I guess that's impractical.

Comment 19

[Mark Banner (:standard8)]

(In reply to Ben Bucksch (:BenB) from comment #12)

Right, see my edited comment 9.

Please don't make major edits to comments after you've posted them. They don't get notified to people and can easily be missed.

You're probably right about the search string. Providers can tokenise it themselves if they want to. It would be nice to have total consistency but I guess that's impractical.

I think we need to consider either providing a library, detailed example or use some other form - that isn't trivial to parse like other query strings. TBH I've always thought that query string format was over engineered for what we actually need to in the address book.

If you have suggestions for a better API, and the API is just as simple as our API, for the simple search-only LDAP-like use case, I'd be happy to see your proposals for comparison.

I've already suggested the calendar provider proposal which was posted before you made the comment. Though I have finally found time to draft out something. I've put it on topicbox as that seems to be the better place for discussion, and others are getting involved as well:

Original thread: https://thunderbird.topicbox.com/groups/addons/Te59c61af548cff18/autocomlete-with-online-data-source
Current thread: https://thunderbird.topicbox.com/groups/addons/T97e89e0d249229e0/autocomplete-with-online-data-source

Comment 20

[[e-mail]]

(In reply to Geoff Lankow from comment #13)

Also, there's no testing.

What testing is available for extension APIs?

I'm not actually sure that you need to create a separate autocomplete provider. As long as you're adding an nsIAbDirectory with useForAutocomplete returning true, I think AbAutoCompleteSearch should be able to handle it. I could be wrong though.

That autocomplete provider still assumes that all searches are fast so that it can get and sort all of the results at once, so it would theoretically work, but it would be very slow, as it doesn't even start the searches in parallel.

> > +    this.propertiesChromeURI = "chrome://messenger/content/addressbook/abAddressBookNameDialog.xhtml";
> > +    this.dirName = String(args.dirName ?? context.extension.name);
> > +    this.dirType = kJsAutoCompleteDirectory;
> > +    this.fileName = "";
> > +    this.UID = String(args.UID ?? Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator).generateUUID().toString().slice(1, -1));

The dirName can be changed through the UI but it's not stored anywhere so it resets as soon as Thunderbird closes.
I don't think there's much point in letting the extension specify the UID.

Could it in theory listen to address book updated events to learn whether the name had been changed?

And you can use AddrBookUtils.newUID to make this tidier.

Thanks for the tip.

Comment 21

[[e-mail]]

Attachment: Trunk patch with some review changes addressed

Comment 22

[Geoff Lankow (:darktrojan)]

Comment on attachment 9200066 [details] [diff] [review]
Trunk patch with some review changes addressed

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

I've just looked at this for the first time in weeks (sorry, busy) and I think it could be simplified by modifying the existing auto-complete handler to do as your new one here does (ie. search all directories concurrently instead of consecutively and having an *_ONGOING result) thus saving the need for a new implementation.

Comment 23

[[e-mail]]

Attachment: Folded into existing autocomplete

Comment 24

[Geoff Lankow (:darktrojan)]

Comment on attachment 9204901 [details] [diff] [review]
Folded into existing autocomplete

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

This is better. The new pieces in AbAutocompleteSearch could use some commentary. I can see at least one problem the linter will complain about, so you need to run that at some point.

You asked about testing and I neglected to answer. See mail/components/extensions/test/xpcshell/test_ext_messages_onNewMailReceived.js for a simple an test you could copy. It's probably enough to set up the event listener inside the background function then do the testing outside using ordinary testing methods.

Comment 25

[Mark Banner (:standard8)]

Please can you explain why this is still a different API to that discussed in the topicbox threads?

Comment 26

[[e-mail]]

Attachment: Added tests

Plus comments and lint.

Comment 27

[[e-mail]]

(In reply to Mark Banner from comment #25)

Please can you explain why this is still a different API to that discussed in the topicbox threads?

This patch isn't intended to be a complete extension address book implementation. However, by providing a mechanism for dynamic transient address book registration, it does at least provide a useful foothold for a future implementation. (As written, it should already be possible for, say, ExQuilla to use it instead of its current hack, but it may be worth tweaking the current patch to make the mechanism more generic than it is right now to further increase its utility.) The patch also provides an immediately useful API that extensions can use to implement an autocomplete result provider, for those cases where they don't want to implement the whole address book.

Comment 28

[Mark Banner (:standard8)]

(In reply to [e-mail] from comment #27)

(In reply to Mark Banner from comment #25)

Please can you explain why this is still a different API to that discussed in the topicbox threads?

This patch isn't intended to be a complete extension address book implementation...<snip>...The patch also provides an immediately useful API that extensions can use to implement an autocomplete result provider, for those cases where they don't want to implement the whole address book.

I don't understand the review request in that case. Whilst I understand the desire to bootstrap and get something in quick, we shouldn't be introducing temporary APIs - deprecation of APIs is always hard, as has already been proven by years of legacy add-ons.

Comment 29

[Ben Bucksch (:BenB)]

This patch has 2 parts:

1. It allows to dynamically add new address book types and implementations from Javascript code. This allows

• to do part 2 below,
• our extensions to remove our ugly hacks where we need to fork the address book manager. (You happened to stumble upon that in the past. Merging this change would allow us to remove that hack and fork in ExQuilla and Owl.)
• it builds the foundation for you to create the address book API that you have in mind.

2. It creates a super-simple API, with a single function, for server-based query-type address book providers.

Possible users are:

• LDAP in JS
• LinkedIn
• Facebook friends
• Company CRM systems
• Owl Exchange company address book search
• you name it
  As a proof of concept, I have created an addon that integrates with Salesforce and allows to search the company customer database. There is a large number of possible data sources that fall into this category, which do not want to sync locally, but only want to answer queries from a server. I would like many addon authors to build such bridges. All these would be able to use this simple one-function API. So, this is not at all "temporary".

I understand that it's not the API that you have in mind, but that doesn't make it less useful. The API that you propose is considerably more complex, and therefore would be both harder to implement, and harder to maintain. And it doesn't exist yet.

An API that consists out of a single function and is useful for a large amount of use cases, is a useful and worthwhile. Even if you added your far more complex, full provider API later on, this API would still be useful. And, this patch even helps you, because it makes it easier to implement your full API.

Comment 30

[[e-mail]]

Attachment: Rebased to trunk

Comment 31

[Mark Banner (:standard8)]

(In reply to Ben Bucksch (:BenB) from comment #29)

I understand that it's not the API that you have in mind, but that doesn't make it less useful. The API that you propose is considerably more complex, and therefore would be both harder to implement, and harder to maintain. And it doesn't exist yet.

The proposed API is not considerably more complex. From an add-on perspective it was basically "Add details of your address book provider", "provide functionality for your provider". I'm pretty sure I suggested previously or on the thread that it could also be not fully implemented to begin with - it could just have the read-only part.

An API that consists out of a single function and is useful for a large amount of use cases, is a useful and worthwhile. Even if you added your far more complex, full provider API later on, this API would still be useful. And, this patch even helps you, because it makes it easier to implement your full API.

If you want to land just the dynamic changes, I'm not against that. Decouple the API from it, so that a proper API can be worked on separately.

I fundamentally disagree with landing the simpler API though - what is the point in having two APIs that fundamentally do the same thing but one is just slightly less complex? Nothing, just more to maintain, and extra confusion for those that want to do similar things.

The point of having the discussions, was to ensure that we had consistent APIs that work for everyone. Persisting with implementing something different disregards those discussions and a process that you participated in designing and by implication agreed to.

Comment 32

[Ben Bucksch (:BenB)]

The proposed API is not considerably more complex.

Yes, it is. It is literally 5 to 10 times more complex.

This API here is still useful, even if you later will have a full address book provider API. I've shown plenty of use cases in my last comment. This API here even allows to remove the C implementation of LDAP and re-implement LDAP in JavaScript. Just the current hookups in the JavaScript code that LDAP needs are already more complex (in lines of code and maintenance) than the API that we add here, so this patch actually reduces the "maintenance burden".

It is not true that more complex use cases are impossible.

• Owl uses exactly this API to implement a search-only address book to query Exchange company address books.
• Owl uses the existing WebExtensions API to implement a fully synced address book, with local cache.
• Exquilla implements the address books on its own (which seems to be what you want). In TB 78, Exquilla currently unfortunately was forced to fork the address book manager. You (Mark) yourself ran into this. I agree that is bad and I hate that we have to do that. This patch fixes it. So, you can implement your own address book provider in JavaScript, but only with this patch.

So, it shows that even though we implement address books in 3 different ways, this API here helps. I am not against a more complex API later, but it's not at all the objective of this bug and what this was about. This is about a search-only address book. This is a 1-function API, which is about as small as it gets.

I'll take questions about the process offline, but suffice to say that we did discuss this on the maililng list in October 2020, and everybody agreed, and the discussion was always specifically about a search-only address book provider like LDAP, and never about a full address book provider, which is something completely different. It's not OK to change the objective after the fact, after we posted the patch here.

Comment 33

[Ben Bucksch (:BenB)]

With this patch landed, Owl can use only public WebExtension APIs for the address book, both

• to implement a fully synced and locally cached address book (which is already possible with current APIs) and
• a search-only address book without cache (which is not currently possible and this patch adds)

If you have different needs, that's a different request and process. This process started on October 2020 was specifically about search-only address books only. There was universal agreement on this. Please do not change the objective of this bug after the fact.

Comment 34

[Mark Banner (:standard8)]

(In reply to Ben Bucksch (:BenB) from comment #32)

I'll take questions about the process offline, but suffice to say that we did discuss this on the maililng list in October 2020, and everybody agreed.

I re-checked the thread and I disagree there was an agreement. It wasn't clear from your comments at the time that this is what you intended. Had an API design been posted at that time or soon after, we'd have been able to discuss it properly. Also no-one did a final summary which didn't help - this bug was referenced but it wasn't clear as to why, it looked like it was responding to a specific comment, not a final "this is what we're intending".

and the discussion was always specifically about a search-only address book provider like LDAP, and never about a full address book provider, which is something completely different.

Even re-reading now it is not clear that everyone was talking about search-only, my comment was definitely not about implementing a separate API just for search-only.

It's not OK to change the objective after the fact, after we posted the patch here.

Again, the intent was not clear until the first patches were posted. If someone posts something completely different to the expectations of those that discussed it, should we accept it just because that's what the person intended?

John, I think you are the owner of the process and probably the de-facto module owner of the APIs, please could you help resolve this?

Comment 35

[John Bieling (:TbSync)]

This is not an easy one. Ben did link this bug in topicbox but it was kind of a drive-by and not a dedicated post for this API. So it is not clear if everybody was able to participate. But most of those community members who DID participate, opposed this approach:

• John (09.01.2021)
• Dirk (10.01.2021)
• Mark (multiple times in this bug)

Nicholas and Jasper are - like everybody - in favor of an API but did not comment on the actual implementation proposal.

Furthermore, there was no public API discussion as requested by the proposal process, which would have sorted this out in a much earlier state.

I have this on my radar, but currently all my time goes into fixing the existing API, trying to get as much done before the next ESR.

As much as I can understand the wish to get away from Experiments (and the "can do everything" permission prompt), I still stand by my statement from January. Let us have a proper API discussion to meet the needs of the community.

Comment 36

[Ben Bucksch (:BenB)]

Hi John,

I think we're talking about different discussion. I was referring to the discussion that happened on September 2020, in thread "Autocomlete with online data source" (sic: Autocomlete) on Thunderbird Addons topic box mailing list.

I will cite a few postings, which gave me the impression that we were all in agreement:

• Original poster wanted to create an addon and could not, due to lack of API (demonstrated need)
• I replied: This "would be super helpful in several situations, and for implementing various protocols"
• John wrote on 2020-09-16: "Contacts autocomplete is a feature for which we definitely need an API."
• Mark Banner wrote on 2020-09-15: "LDAP does this already, so this case could just be another 'on-demand' style address book"
• That last quote was particularly encouraging for me, because Mark usually disagrees with me, and here I agreed 100%.
• I (Ben Bucksch) replied on 2020-09-17 in response to Mark: "Exactly that's what I had in mind, yes. The API should be an 'search-style' address book, which would also be usable other places in the UI, e.g. from the address book window and the calendar invitees etc."

I know that Mark now says that he didn't mean it in the way I understood it, but re-reading it even now, I definitely understand John's, Mark's and my own comment so that there would be a address book API that is specific for search-only address books, not that there would be a generic API for full address book providers.

Encouraged by this universal agreement, and I decided that we wanted to contribute that API. It wasn't possible to post the concrete API, because we didn't know it at this time. However, I still believe that the API here is very much faithful to what we had discussed and what I quoted above. This patch allows a WebExtension to implement an LDAP-like search-only address book, usable in autocomplete and in the address book window, by implementing only a single function.

We said that the implementation should happen in bugzilla, and we followed that, and that's exactly what we did. We had posted the implementation and API here in this bug, and posted the link on topicbox, not in passing, but every explicitly on 202-10-12 posted the link to this bug here. (I didn't write more text, because it's my form of understatement, and also to avoid duplication, because the explainations are already in bugzilla.) The patch was available at that time, 6 months ago, as you can see here in this bug in the obsolete patches. The fact that everybody ignored it and nobody bothered to look at this bug and its patch shouldn't be to our disadvantage.

3 months later, Mark posted his disagreement here, and demanded that we implement a full address book provider API. However, that was never the purpose of this bug, and I would not have accepted this task, because it's too much work, there are a lot of API details to work out, and it costs too much discussion to finalize. And we simply have no need for that, neither did the original poster who originally requested this API. It wasn't the purpose here, it's a different use case. If Mark wants to create such an API, that's fine with me, but we shouldn't have to wait for that, nor be forced to implement something that we have no need for. I also disagree with Mark that this API we create here is temporary or hard to maintain. Neither is true, but the API here is useful long-term, for many use cases.

We followed the process (at that time!) to the letter:

1. Post on addons mailing list, showing the need. (Sept 2020)
2. General discussion about approach, on the mailing list (Sept 2020)
3. Implementation and finalization of API (and posted on bugzilla and referenced on mailing list) (Oct 2020)
4. Review and landing.
   The disagreement came in step 4, in Jan 2021, a more than 3 and a half months after implementation was finished.

Comment 37

[Ben Bucksch (:BenB)]

FWIW, so that we don't all miss the point. Here is the API we propose:

messenger.addressBooks.onSearchRequest.addListener(async searchString => {
  let response = await fetch("https://....?query=" + searchString);
  // return response.json.map(contact => { DisplayName: contact.name, PrimaryEmail: contact.email });
  return [ { DisplayName: "Will Doe", PrimaryEmail: "[e-mail]" }, ... ];
}, {
 dirName: "LinkedIn",
 isSecure: true,
 UID: "00e1d9af-a846-4ef5-a6ac-15e8926bf6d3",
});

That's it. That's all we need. I really don't see the point how one can compare that to a full address book provider API.

We will separate the patch into 2, the first patch containing the general ability in address book manager to add an address book, and general fixes to remove assumptions that the code makes about LDAP, and a second patch which adds the new WebExtension API.

Comment 38

[[e-mail]]

Attachment: Backend only without Extension API

(Also rebased again)

This just contains the backend code that allows an address book to be added with support for asynchronous autocompletion from it.

Comment 39

[[e-mail]]

Attachment: Linted patch

Ugh, you do insist on making the code less readable...

Comment 40

[[e-mail]]

Attachment: Extension search request listener API only

Comment 41

[Ben Bucksch (:BenB)]

Attachment 9220169 [details] [diff] solves a few aspects, which are necessary to fix this properly:

• It adds the function addAddressBook to give the ability to add an address book implemented by the provider. The old API only has a newAddressBook, which creates a new address book using a built-in implementaiton (local etc.).
• The current code makes assumptions about the results. It assumes that results from local address books are complete, e.g. searching for "Je" will return all results that start with "Je", whereas it assumes that providers that give async results return incomplete results, therefore searching "Jef" after "Je" will trigger a new search. This is particularly expensive for server calls, which will always be async. We are adding a property where the provider can declare whether all results were returned or only a partial result. This allows to give much faster results for server-based providers while typing into the autocomplete widget. This makes a difference once the user types first "Je" and then "f" (for "Jef"): If "Je" already returned all results, we do not need to make a new server query, but can filter the known results. The current address book code already has logic to do exactly that optimization. The only problem is that it makes that assumption about which providers provide complete and incomplete results. We only add the new boolean flag which removes that assumption and simply makes it explicit. Given that we now can add new providers, we no longer can make an assumption.
• When the user enters "Jeff Barns", the code constructs a complicated LDAP-style query from that, which basically says "Jeff AND Barns OR Jeff OR Barns", and it's impossible to get to the original search string that the end user entered. That makes it exeedingly difficult for providers where the server already implements that logic. We are adding a parameter to the search function that passes along the user's original search string, so the provider can optionally use that to make the search. This is required for our implementation, because Exchange doesn't use nor understand LDAP-style queries. Same for pretty much all other providers and search servers.

Comment 42

[Ben Bucksch (:BenB)]

Attachment 9220169 [details] [diff] are the parts that allow other providers to add address books. It does not include the MailExtension API that we propose. For example ExQuilla needs this patch, but not the rest. This is the generic part, independent from the MailExtension API, which just allows to add a non-core address book implementation.

Attachment 9220194 [details] [diff] adds the MailExtension API for a search-only address book.
This could be used to implement LDAP as a MailExtension instead of in core. Likewise, we use that in Owl to implement the Exchange corporate address book, and I used this API to implement a SalesForce addon that uses the Salesforce customer database. These are available in autocomplete, and they also show up in the address book window, as a normal address book, but showing entries only after a search, exactly like LDAP behaves.

Comment 43

[rsjtdrjgfuzkfg (Dirk Steinmetz)]

I had a short look at the registration part of Attachment 9220169 [details] [diff]. There are a few codestyle issues I'm not sure about, most notably

• ASYNC_TYPE should probably be ASYNC_DIRECTORY_TYPE for consistency
• Surrounding code usually keeps if-conditions and method arguments in a single line, while this patch often spreads them out multiple, very short lines.
• newAddressBook could use the new addAddressBook method to avoid further code duplication (the existing duplication is not the patch's fault, but it seems natural to fix it here)

That being said, I think we should merge something along Attachment 9220169 [details] [diff] very soon, to permit all address-book related add-ons to switch to better experiments for the next ESR release (where rewrites are necessary anyway due to e10s). Getting a method to add address books is essential for any clean address book provider API (including experiments for individual add-ons) and would be very useful for future core API development (as it permits readable draft API implementations, either as patch or as experiment).

I cannot really comment on the autocompletion abstraction (the ASYNC_TYPE-specific handling stuff), but would trust on Ben's experience to understand what various providers could need here. So unless somebody that also maintains an add-on / core feature in that area feels the need to weigh in, I'd be in favor of merging that as well.

Comment 44

[Ben Bucksch (:BenB)]

Surrounding code usually keeps if-conditions and method arguments in a single line, while this patch often spreads them out multiple, very short lines.

That's the linter, see comment 39.

Getting a method to add address books is essential for any clean address book provider API (including experiments for individual add-ons) and would be very useful for future core API development

Exactly. Splitting this into 2 patches is a compromise, to get something moving forward.

we should merge something along Attachment 9220169 [details] [diff] very soon, to permit all address-book related add-ons to switch to better experiments for the next ESR release

Thanks! Yes, that's the goal. For ExQuilla for TB 78, because all address book implementations were hardcoded in TB 78, with no ability to add one, we were currently forced to fork (!) and replace the entire TB-internal address book manager. Also now in Owl, for the search-only address book. For TB 91, I really really want that fork gone. This patch is required to solve it. So, this solves a real and urgent problem.

ASYNC_TYPE should probably be ASYNC_DIRECTORY_TYPE for consistency

No problem. We're happy to fix nits.

Comment 45

[rsjtdrjgfuzkfg (Dirk Steinmetz)]

(In reply to Ben Bucksch (:BenB) from comment #44)

That's the linter, see comment 39.

Whoops. Missed that part. The people deciding on the linter settings should imho do a mass-linting commit then. The mix is not really good for readability. But not related to this bug.

because all address book implementations were hardcoded in TB 78, with no ability to add one, we were currently forced to fork (!) and replace the entire TB-internal address book manager. Also now in Owl, for the search-only address book.

Monkey-patching the address book provider is possible, I do that in GeneralSync. But it is very ugly.

That being said, GeneralSync's current monkey-patch also permits persistence (so address book reloads are handled by the manager, not the experiment). Would be nice to have, but the more I think about edge cases the more I think that it is probably better to bite the bullet and to use "wrong" implementations with built-in schemas (and thus worse startup/reload performance) and not registering add-on address books in the preferences.

The migration will be a nightmare, though (again)... ;)

Comment 46

[Geoff Lankow (:darktrojan)]

I think we can compromise here to get this patch across the line. What I suggest is:

• When the event fires it passes an AddressBookNode object as the first parameter. This would be the same directory as created when the event listener is added, which does seem pointless bug it brings this event into line with the cloudFile API events and those of the proposed calendar provider API. It also enables the future expansion without having to modify the parameters.
• In the future, we can make the extra parameter to addListener optional, and if not given, not create a directory, but rather use an existing directory created by some other means yet to be determined. This is where the AddressBookNode parameter may come in handy.

Does that satisfy (or at least not block) everybody's needs?

Comment 47

[Mark Banner (:standard8)]

(In reply to Geoff Lankow (:darktrojan) from comment #46)

I think we can compromise here to get this patch across the line. What I suggest is:

• When the event fires it passes an AddressBookNode object as the first parameter. This would be the same directory as created when the event listener is added, which does seem pointless bug it brings this event into line with the cloudFile API events and those of the proposed calendar provider API. It also enables the future expansion without having to modify the parameters.
• In the future, we can make the extra parameter to addListener optional, and if not given, not create a directory, but rather use an existing directory created by some other means yet to be determined. This is where the AddressBookNode parameter may come in handy.

Does that satisfy (or at least not block) everybody's needs?

I think overall this is a reasonable way forward as long as the API is marked as unstable. John (:TbSync) has said to me this is the intention, but I've not seen that referenced anywhere.

Additionally I would recommend that:

• The new API is not in the browser.contacts.* namespace, but a new namespace. The reason being that when a provider API is implemented, there will then be a mixture of contact retrieval APIs and provider APIs in the same namespace and I can see that being confusing. We should separate them now, so that it is clear, and less potential churn later.
• Use name for the address book name, or something like that. dirName is exposing an old internal detail that isn't necessary for WebExtensions to know about. This would be more consistent with AddressBookNode.
• Glancing at the patch, I noticed that the UID parameter of the API does not have any validation on it - so it could potentially conflict with a user's existing address book. Although this is unlikely, I think there should be validation done.
  • Even better, the API could generate the UID itself - I see no reason for it to be supplied by the add-on at this stage.
  • If kept, it should also just be id to be consistent with AddressBookNode.
• The API patch should have tests to check that duplicate address book names (and IDs if appropriate) are correctly rejected by the API.

Comment 48

[[e-mail]]

Attachment: With ASYNC_DIRECTORY_TYPE as requested in comment #43

Comment 49

[[e-mail]]

Attachment: Addressed review comments

Comment 50

[[e-mail]]

Comment on attachment 9223788 [details] [diff] [review]
Addressed review comments

There have been some address book changes on trunk so I'll need to update this patch.

Comment 51

[[e-mail]]

Comment on attachment 9223787 [details] [diff] [review]
With ASYNC_DIRECTORY_TYPE as requested in comment #43

(Ditto.)

Comment 52

[[e-mail]]

Comment on attachment 9223787 [details] [diff] [review]
With ASYNC_DIRECTORY_TYPE as requested in comment #43

Actually this one wasn't affected. Sorry for the noise.

Comment 53

[[e-mail]]

Attachment: With test for duplicate id

For some reason throwing an exception in an event listener registration function doesn't propagate to the extension, so I had to test this by asserting that the second registration is simply ignored.

Comment 54

[Ben Bucksch (:BenB)]

@Geoff: Ready for re-review

Comment 55

[Geoff Lankow (:darktrojan)]

Comment on attachment 9223787 [details] [diff] [review]
With ASYNC_DIRECTORY_TYPE as requested in comment #43

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

Looks good to me but some other things need updating too. Sorry about the delay, whenever I attempt to start this review I get sidetracked by more urgent things.

::: mailnews/addrbook/content/abResultsPane.js
@@ -78,5 @@
>    }
>  
>    gAbView = gAbResultsTree.view = new ABView(
>      GetDirectoryFromURI(aURI),
>      aSearchQuery,

Same change required in mail/components/addrbook/content/aboutAddressBook.js (x3).

::: mailnews/addrbook/content/abView.js
@@ +7,5 @@
> +function ABView(
> +  directory,
> +  searchQuery,
> +  searchString,
> +  listener,

Same changes required in mail/components/addrbook/content/abView-new.js.

::: mailnews/addrbook/modules/AddrBookManager.jsm
@@ +407,5 @@
> +    ) {
> +      throw new Components.Exception(
> +        "Invalid autocomplete directory",
> +        Cr.NS_ERROR_INVALID_ARG
> +      );

This error message should be more generic – we might use the function for non-autocomplete directories.

::: mailnews/addrbook/public/nsIAbAutoCompleteResult.idl
@@ +45,5 @@
> +
> +  /**
> +   * Array of asynchrouns address books that were unable to return full results.
> +   * This means that they need to be requeried rather than simply filtered.
> +   */

Typo.

::: mailnews/addrbook/public/nsIAbDirSearchListener.idl
@@ +35,5 @@
>     *                 LDAP servers with self-signed certs).
>     * @param location If status is an NSS error code, this holds the location
>     *                 of the failed operation ("<host>:<port>").
>     */
> +  void onSearchFinished(in nsresult status, in bool complete, in nsITransportSecurityInfo secInfo, in ACString location);

Please update mailnews/addrbook/modules/LDAPDirectoryQuery.jsm for this change.

::: mailnews/addrbook/public/nsIAbManager.idl
@@ +74,5 @@
> +   * Adds a previously created address book objet.
> +   *
> +   * @param aDir The address book object.
> +   *
> +   * The address book does not persist between sessions.

Typo in the first line, and also this last line should be a part of one of the other lines. Something like: "Adds a previously created address book object until it is removed (using deleteAddressBook) or the end of the session."?

Comment 56

[Geoff Lankow (:darktrojan)]

Comment on attachment 9224170 [details] [diff] [review]
With test for duplicate id

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

I think Mark is right, let's put this in `addressBooks.provider`. On the whole, this is getting close to ready.

::: mail/components/extensions/parent/ext-addressBook.js
@@ +27,5 @@
>  /**
> + * Address book that supports finding cards only for a search (like LDAP).
> + * @implements {nsIAbDirectory}
> + */
> +class ExtSearchBook {

You can extend AddrBookDirectory, implement the abstract methods, and skip most of this now.

::: mail/components/extensions/schemas/addressBook.json
@@ +246,5 @@
> +      },
> +      {
> +        "name": "onSearchRequest",
> +        "type": "function",
> +        "description": "Fires when searching for a contact.",

You need to say that a new read-only address book is created using the values in the parameters object. I don't think that somebody who stumbles across this API would be expecting that.

You should also say that the exact way this event works may change in a future release.

@@ +255,5 @@
> +          },
> +          {
> +            "name": "searchString",
> +            "type": "string"
> +          },

Needs a description saying this is the text the user entered.

I also notice this is null when using the advanced address book search dialog. You'll have to deal with that case somehow, either by not firing the event at all, or by marking this param as optional and saying why it might be missing in the description.

@@ +260,5 @@
> +          {
> +            "name": "query",
> +            "type": "string",
> +            "optional": true
> +          }

Needs a description saying what this is. You should also say that it's subject to change, because it is (I'm planning such a change after 91.)

@@ +281,5 @@
> +              "id": {
> +                "type": "string",
> +                "optional": true,
> +                "description": "The id of the address book."
> +              }

I see no need for the add-on to control the ID.

::: mail/components/extensions/test/xpcshell/test_ext_addressBook_onSearchRequest.js
@@ +107,5 @@
> +
> +registerCleanupFunction(() => {
> +  // Make sure any open database is given a chance to close.
> +  Services.obs.notifyObservers(null, "quit-application");
> +});

You need to test that removing the listener removes the directory and that stopping the extension removes the directory.

Comment 57

[Ben Bucksch (:BenB)]

Hey Geoff, thanks so much for the review!

I see no need for the add-on to control the ID.

We need to keep track of the ID in our addon. It was easier to allow to pass it in, than to return it. (The parameter is optional and the default is that TB creates the ID automatically, so you can ignore it, if you don't need it.)

All the other review comments are good. Stay tuned.

Comment 58

[[e-mail]]

Attachment: Addressed review comments

Same changes required in mail/components/addrbook/content/abView-new.js.

Mercurial actually did that for me. (Normally I don't like that, but I guess it saved me some effort this time.)

Comment 59

[[e-mail]]

Attachment: Addressed review comments

put this in addressBooks.provider

I hope this is what you meant, if not please let me know what you actually meant?

skip most of this now.

Well, some of the savings were eaten up with having to override stuff...

I also notice this is null when using the advanced address book search dialog.

Great catch, thanks. I added an explicit parameter in that dialog to clarify what was going on.

You need to test that removing the listener removes the directory and that stopping the extension removes the directory.

Surely Gecko tests that stopping extensions removes listeners? (But I added one anyway.)

Comment 60

[[e-mail]]

Try push: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=6d063a51996ea9201365aa3e41855c2ee2d5ff55

Comment 61

[Geoff Lankow (:darktrojan)]

Comment on attachment 9226596 [details] [diff] [review]
Addressed review comments

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

On track, just need the code in the right places now.

::: mail/components/extensions/schemas/addressBook.json
@@ +244,5 @@
>            }
>          ]
> +      },
> +      {
> +        "name": "provider",

No that isn't what I meant. At the top level of this file are four objects corresponding to API namespaces: manifest, addressBooks, contacts, mailingLists. Create a fifth one named addressBooks.provider and move this event to it. In the return value of the `getAPI` method in ext-addressBooks.js you'll need to do the same, add another member called addressBooks.provider and move your event there. I hope that makes sense.

Your code will end up calling `browser.addressBooks.provider.onSearchRequest…`.

@@ +261,5 @@
> +          },
> +          {
> +            "name": "query",
> +            "type": "string",
> +            "description": "The LDAP query string corresponding to the search. Note: This parameter may change in future releases of Thunderbird.",

You mean Lisp query string (or just "The query string" although that gives no hints at all about how to use it). It's not related to LDAP.

Comment 62

[[e-mail]]

(In reply to Geoff Lankow from comment #61)

You mean Lisp query string. It's not related to LDAP.

https://www.google.com/search?q=%22lisp+query+string%22 says "It looks like there aren't many great matches for your search".

On the other hand, https://www.google.com/search?q=%22ldap+query+string%22 gives fairly relevant search results.

Comment 63

[[e-mail]]

Attachment: addressBooks.provider.onSearchRequest

add another member called addressBooks.provider and move your event there

I originally thought you literally meant for me add a member called addressBooks.provider but it turns out that this crashes debug builds and what you actually meant was add a provider member to the existing addressBooks API object. (Or at least I hope you meant that, because that's what works.)

Comment 64

[Geoff Lankow (:darktrojan)]

(In reply to [e-mail] from comment #62)

(In reply to Geoff Lankow from comment #61)

You mean Lisp query string. It's not related to LDAP.

https://www.google.com/search?q=%22lisp+query+string%22 says "It looks like there aren't many great matches for your search".

On the other hand, https://www.google.com/search?q=%22ldap+query+string%22 gives fairly relevant search results.

That might be the case, but it's not an LDAP query string so the results for that are irrelevant. This is about the only page that explains it, unfortunately. You could link straight to that. I intend to replace the string format with something more sensible as there's no reason for us to use it any more.

(In reply to [e-mail] from comment #63)

Created attachment 9227905 [details] [diff] [review]
addressBooks.provider.onSearchRequest

add another member called addressBooks.provider and move your event there

I originally thought you literally meant for me add a member called addressBooks.provider but it turns out that this crashes debug builds and what you actually meant was add a provider member to the existing addressBooks API object. (Or at least I hope you meant that, because that's what works.)

I did mean that but obviously I got it wrong. Looks like you've figured it out though.

Comment 65

[Geoff Lankow (:darktrojan)]

Comment on attachment 9227905 [details] [diff] [review]
addressBooks.provider.onSearchRequest

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

You haven't updated the test since moving the event. I'm gonna start a Try run with that fixed since your previous Try run didn't build or run any tests.

Comment 66

[Geoff Lankow (:darktrojan)]

Try choked on Mac and Windows. Looks like you need to update nsAbOSXDirectory.h and nsAbOutlookDirectory.h as well as the .cpp files.

Comment 67

[Geoff Lankow (:darktrojan)]

Comment on attachment 9226595 [details] [diff] [review]
Addressed review comments

As well as the compilation failures there's test failures in

• mail/components/addrbook/test/browser/browser_ldap_search.js
• mail/components/addrbook/test/browser/new/browser_ldap_search.js
• mailnews/addrbook/test/unit/test_ldapReplication.js
• mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch2.js

The other failures are known problems. Debug tests haven't finished when I wrote this but I don't expect anything different there.

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=5bbad2c3c0c472ec244fe5f1b82092d2c60a8596

Comment 68

[Ben Bucksch (:BenB)]

Geoff wrote in comment 64:

I intend to replace the [query] string format with something more sensible

Yes, please! :) Thank you.

Comment 69

[[e-mail]]

Attachment: Updated for new LDAPDirectory.jsm

It needed a related change to fix three tests. There's also the fix for the fourth test failure.

Comment 70

[[e-mail]]

(also potential fixes for the compilation failures, but the try run isn't starting... rebase time again?)

Comment 71

[[e-mail]]

Attachment: Updated test

(That syntax looks as much like LISP as real LDAP syntax does, so I've avoided mentioning either for now.)

Comment 72

[[e-mail]]

Try push: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=ff2aa2a674aa6d3d8dc9025d766dff1f670985a1

Comment 73

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a025ba535f5a
Backend for asynchronous addressbook searches. r=darktrojan
https://hg.mozilla.org/comm-central/rev/87d34cfcf232
Extension API to provide contacts on demand when searched. r=darktrojan

Comment 74

[Ben Bucksch (:BenB)]

Thank you, Geoff!

:-)

Comment 75

[John Bieling (:TbSync)]

@Ben,
@Neil,

I added the documentation for the new addressBooks.provider API to
https://webextension-api.thunderbird.net/en/latest/

Since I am not that familiar with it, would you have the time to provide a few descriptive sentences? This is the commit which added it, but includes placeholders:
https://github.com/thundernest/webext-docs/commit/dc19d0773560fdcf9b46af06bdd3e30fe40e8023

These two files are source files (the others are generated):

• index.rst
• overlay/addressBooks.provider.rst

and both contain a <AB.PROVIDER.TEXT> placeholder. The intro text in addressBooks.provider.rst should include, when it was added to the API.

The generator also allows to add sample code, I did that for example here:
https://thunderbird-webextensions.readthedocs.io/en/latest/messages.html#getraw-messageid

If you want to enrich the documentation with code snippets, just add the files in your PR here
https://github.com/thundernest/webext-docs/tree/master/includes

You can either tell me where to add them, or prepare the overlay files on your own. Compare the <literalinclude> tags here:
https://github.com/thundernest/webext-docs/blob/master/overlay/messages.json#L31

Thanks for your help,
John

Comment 76

[Ben Bucksch (:BenB)]

Thanks, John.

I've pushed a description, explanation, guide and sample code to PR 55.