1469238 - Address Book WebExtensions API

Status: RESOLVED FIXED

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

Description

[Geoff Lankow (:darktrojan)]

This is my first attempt at making a WebExtensions API specifically for Thunderbird. It's not complete, nor is it all that polished, but here we are.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1469238-webext-addressbook-1.diff

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8985887 [details] [diff] [review]
1469238-webext-addressbook-1.diff

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

This is a good start! Some comments inline:

::: mail/components/extensions/parent/ext-addressBook.js
@@ +42,5 @@
> +        getAll: async function() {
> +          return [...fixIterator(MailServices.ab.directories, Ci.nsIAbDirectory)].map(ab => {
> +            return {
> +              name: ab.dirName,
> +              uri: ab.URI

Looking at some of the other WX apis, it may make sense to make this a separate class instead of a plain object, and possibly identify addressbooks with a numeric id instead of the URI (kind of like the tab/window apis)

@@ +57,5 @@
> +        },
> +        delete: function(uri) {
> +          MailServices.ab.deleteAddressBook(uri);
> +        },
> +        getMailingLists: async function(uri) {

Maybe makes sense for mailing lists and contacts to also be separate (exposed) classes, analog to how the windows api does not provide functions to modify tabs within the window.

@@ +115,5 @@
> +          }
> +          if (card) {
> +            // TODO: need a way to remove unwanted properties,
> +            // but unspecified values are null in newProperties.
> +            // Should we require all properties to be returned?

If you make a contact a separate class, you could keep track of dirty state.

::: mail/components/extensions/schemas/addressBook.json
@@ +24,5 @@
> +          "HomeCity": {"optional": true, "type": "string"},
> +          "HomeState": {"optional": true, "type": "string"},
> +          "HomeZipCode": {"optional": true, "type": "string"},
> +          "HomeCountry": {"optional": true, "type": "string"},
> +          "WebPage2": {"optional": true, "type": "string"},

I'm not sure what the best approach is to handle property names. The way it is now, it seems like we are exposing the nsIAbCard names directly, which is already an implementation detail due to how the address book works.

What I'd like to see, and I am not sure it is possible, is to design the address book and contact properties on a high level, then see how nsIAbCard fits in. For example, instead of having WebPage1 and WebPage2 there should probably be a way to add more than one web pages, with the internal limitation that it can only be 2 for now.

@@ +52,5 @@
> +          "Custom3": {"optional": true, "type": "string"},
> +          "Custom4": {"optional": true, "type": "string"},
> +          "Notes": {"optional": true, "type": "string"},
> +          "_GoogleTalk": {"optional": true, "type": "string"},
> +          "_AimScreenName": {"optional": true, "type": "string"},

Similar about IM names, and likely a few others here. Also, _AimScreenName is duplicate.

@@ +153,5 @@
> +      },
> +      {
> +        "name": "delete",
> +        "type": "function",
> +        "async": "callback",

async: "callback" is (afaik) a compatibility thing to also support Chrome callback variants of these functions. I think we can just go with promises only given there is no other API we have to be compatible with.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8985887 [details] [diff] [review]
1469238-webext-addressbook-1.diff

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

::: mail/components/extensions/schemas/addressBook.json
@@ +137,5 @@
> +        "async": "callback",
> +        "parameters": [
> +          {
> +            "type": "string",
> +            "name": "uri"

I'm concerned about exposing the uri for address books, cards & mailing lists - those are really specific to the backend, and I'd doubt that any future implementation would want to support them in the same way.

I would strongly recommend you switched to guids. Similar to what Firefox's places database does. They use unique guid strings, plus some of the top-level folders have specific defined guids attached so they can always be easily referenced.

It'd be more work to make the existing address book support it (or provide some sort of consistent interface), but I think it could be worth it to avoid breaking APIs in the long term (which is one of the ideas for WebExtensions...).

Comment 4

[Geoff Lankow (:darktrojan)]

(In reply to Mark Banner (:standard8) from comment #3)
> I'm concerned about exposing the uri for address books, cards & mailing
> lists - those are really specific to the backend, and I'd doubt that any
> future implementation would want to support them in the same way.
> 
> I would strongly recommend you switched to guids. Similar to what Firefox's
> places database does. They use unique guid strings, plus some of the
> top-level folders have specific defined guids attached so they can always be
> easily referenced.
> 
> It'd be more work to make the existing address book support it (or provide
> some sort of consistent interface), but I think it could be worth it to
> avoid breaking APIs in the long term (which is one of the ideas for
> WebExtensions...).

You're right, I don't really like it either, at this point it's just a way to uniquely identify things, so we/I can get an idea of how an extension might interact with the API.

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: 1469238-webext-addressbook-2.diff

This is still pretty rough, but I thought it about time I posted some progress.

I've split the API up into three namespaces: addressBooks, contacts, mailingLists. It's a little bit weird because mailing lists are basically address books contained inside an address book. The contacts methods work with either.

I've also stopped using URIs everywhere and keep an ID for the address books/mailing lists. (The function for making IDs is just good enough for now, I don't intend to keep it that way.) I'd like to be able to make the ID persistent, as I think there is a good chance extensions might want to keep it in a pref and refer to it later. I'm not sure how I should do that without changing the interface. Prefs, maybe?

Comment 6

[Mark Banner (:standard8)]

(In reply to Geoff Lankow (:darktrojan) from comment #5)
> I've split the API up into three namespaces: addressBooks, contacts,
> mailingLists. It's a little bit weird because mailing lists are basically
> address books contained inside an address book. The contacts methods work
> with either.

It might be worth looking at the Bookmarks API from Firefox. We have different types of items represented in the database (bookmarks, folders, separators), but they are all represented as a node: https://developer.mozilla.org/docs/Mozilla/Add-ons/WebExtensions/API/bookmarks

Mailing lists were always a bad hack, and really should be a simple list of ids of other cards. Although we might really need to make that a list of [addressBook guid, card guid] pairs.

> I've also stopped using URIs everywhere and keep an ID for the address
> books/mailing lists. (The function for making IDs is just good enough for
> now, I don't intend to keep it that way.)

You could potentially base it on history's version: https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/toolkit/components/places/Helpers.cpp#243

> I'd like to be able to make the ID
> persistent, as I think there is a good chance extensions might want to keep
> it in a pref and refer to it later. I'm not sure how I should do that
> without changing the interface. Prefs, maybe?

I think we should just do the work and add guid to the address book database. That way we'd always have a consistent reference to an item in the database. If it gets rewritten, then we can still reference by guid, it could even help to make mailing lists more reliable. Prefs would be a really expensive way to do this.

Comment 7

[Magnus Melin [:mkmelin]]

Regarding generating the guid, please use a v4 guid. You can generate it using 

function uuidv4() {
  return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, function(c) {
    var r = Math.random() * 16 | 0, v = c == 'x' ? r : (r & 0x3 | 0x8);
    return v.toString(16);
  });
}

[From https://stackoverflow.com/questions/105034/create-guid-uuid-in-javascript]

Comment 8

[Mark Banner (:standard8)]

(In reply to Magnus Melin from comment #7)
> Regarding generating the guid, please use a v4 guid.

What's the reason for needing so many bits of entropy? String comparisons are expensive, especially with database lookups, so I think we should be looking to have some sort of balance between entropy and string length. For example, on the whole I suspect the places database will store more items, but currently uses a 12 character guid.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 8998135 [details] [diff] [review]
1469238-webext-addressbook-2.diff

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

::: mail/components/extensions/parent/ext-addressBook.js
@@ +134,5 @@
> +              }
> +            }
> +            directory.modifyCard(card);
> +          } else {
> +            throw "Found nothing";

please avoid throwing strings. Always throw new Error("<message here>")

Comment 10

[Magnus Melin [:mkmelin]]

(In reply to Mark Banner (:standard8) from comment #8)
> (In reply to Magnus Melin from comment #7)
> > Regarding generating the guid, please use a v4 guid.
> 
> What's the reason for needing so many bits of entropy? String comparisons
> are expensive, especially with database lookups, 

Databases don't do lookup by the string anyway, but on their internal hash of it, right?

You could imagine use cases of perhaps having ways of sharing items with others, and to support any use cases outside of the local computer, you're going to end up with using a guid (v4).

Comment 11

[Mark Banner (:standard8)]

(In reply to Magnus Melin from comment #10)
> Databases don't do lookup by the string anyway, but on their internal hash
> of it, right?

I suspect that depends on how the database is configured. There's still an amount of string comparison + management to do at some stage.

> You could imagine use cases of perhaps having ways of sharing items with
> others, and to support any use cases outside of the local computer, you're
> going to end up with using a guid (v4).

Places does just fine with those 12 characters as far as we know. That's including handling Sync. 

Personally, I think a v4 is overkill here, but *shrug*.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1469238-addressbook-guids-1.diff

This is foreign territory for me, so am I doing this right?

I'm trying to add a GUID property to top-level address books (stored in the prefs), mailing lists, and contacts (both stored in the Mork DB). I'll fill in the values later, only if the user has an extension that uses this API.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8998719 [details] [diff] [review]
1469238-addressbook-guids-1.diff

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

I'm not sure what I'm giving feedback on here. What it the GUID needed for? Did you want me to see whether it's correctly passed around everywhere? I didn't do that.

::: mailnews/addrbook/public/nsIAbCard.idl
@@ +108,5 @@
>     * implementations SHOULD NOT, in general, modify this property.
>     */
>    attribute AUTF8String localId;
>  
> +  attribute ACString GUID;

Hmm, why ACString? Wouldn't a plain 'string' do it? Do we expect more than ASCII? Maybe you want to map to the more modern String interface. Besides, "attribute ACString" is widely used, so OK.

::: mailnews/addrbook/public/nsIAddrDatabase.idl
@@ +207,5 @@
>    [noscript] readonly attribute nsIMdbRow newRow;
>    [noscript] readonly attribute nsIMdbRow newListRow;
>    [noscript] void addCardRowToDB(in nsIMdbRow newRow);
>    [noscript] void addLdifListMember(in nsIMdbRow row, in string value);
> +  [noscript] void addGUID(in nsIMdbRow row, in string value);

This is a little inconsistent with the 'attribute ACString', no? Would it be better to use ACString here?

Comment 14

[Mark Banner (:standard8)]

(Ideally, I think the patch should be on a separate bug, so we can keep this one just about the WebExtension interface).

(In reply to Jorg K (GMT+2) from comment #13)
> > +  attribute ACString GUID;
> 
> Hmm, why ACString? Wouldn't a plain 'string' do it? Do we expect more than
> ASCII? Maybe you want to map to the more modern String interface. Besides,
> "attribute ACString" is widely used, so OK.

ACString is ASCII only, but a much nicer interface than char* (aka 'string'). AUTF8String or AString are the non-ascii versions (wstring is the non-ascii version using old style string pointers).

Comment 15

[Geoff Lankow (:darktrojan)]

Comment on attachment 8998719 [details] [diff] [review]
1469238-addressbook-guids-1.diff

(In reply to Mark Banner (:standard8) from comment #14)
> (Ideally, I think the patch should be on a separate bug, so we can keep this
> one just about the WebExtension interface).

You're right. Bug 1482040.

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: 1469238-webext-addressbook-3.diff

This is still a work in progress, but it's much closer to completion than before.

I've borrowed a few tricks from the bookmarks API as suggested, but I'm not really happy pretending there's a tree structure here (despite the fact I've used the word everywhere).

I also haven't figured out what to do with contact properties. I might just put them in the schema as "object" and document the list of properties Thunderbird supports.

Finally, some events on this API would be handy, but this is proving tricky as the notifications from the address book backend are a bit useless. For example, when an address book is removed, the GUID and URI properties are already empty, so I can't use them.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 8999482 [details] [diff] [review]
1469238-webext-addressbook-3.diff

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

::: mail/components/extensions/parent/ext-addressBook.js
@@ +95,5 @@
> +    this._tree = null;
> +  },
> +  generateGUID() {
> +    const GUID_LENGTH = 12;
> +    return btoa(String.fromCharCode(...crypto.getRandomValues(new Uint8Array(GUID_LENGTH / 4 * 3))));

I'd still go for the v4 guid as mentioned earlier.

About the naming: it is a guid, but we could name property uid, matching vCard's naming.

@@ +102,5 @@
> +    function checkNode(parentNode) {
> +      if (type == parentNode.type && id == parentNode.id) {
> +        return parentNode;
> +      } else if (type == "contact") {
> +        return parentNode.contacts.find(c => id == c.id);

no else after return please

@@ +343,5 @@
> +          let node = cachedTree.findObjectByGUID("contact", id);
> +          let parentNode = cachedTree.findObjectByGUID("addressBook", node.parentId);
> +          // TODO: need a way to remove unwanted properties,
> +          // but unspecified values are null in newProperties.
> +          // Should we require all properties to be returned?

Seems reasonable that the passed in object would be the old contact one with modifications done.

Maybe name the function update? That would be nice in the other places too, you know, CRUD. That way people don't need to think about what the methods might be named.

::: mail/components/extensions/schemas/addressBook.json
@@ +153,5 @@
> +      {
> +        "id": "ContactProperties",
> +        "type": "object",
> +        "properties": {
> +          "FirstName": {"optional": true, "type": "string"},

Instead of using the thunderbird internal properties it would be nice to expose the properties mapped to vcard.

Comment 18

[Geoff Lankow (:darktrojan)]

Attachment: 1469238-webext-addressbook-4.diff

Comment 19

[Geoff Lankow (:darktrojan)]

Attachment: 1469238-webext-addressbook-5.diff

Now with permissions I forgot to add, and the indentation not stuffed-up.

Comment 20

[Geoff Lankow (:darktrojan)]

Attachment: 1469238-webext-addressbook-6.diff

I think it is time this API happened. There are a few recent changes, most notably that I've started blacklisting contact properties that an extension can't see or use, instead of whitelisting only the ones that Thunderbird uses. The database doesn't care what is put in it, so neither should the API.

Comment 21

[Mark Banner (:standard8)]

I like where this is heading. Whilst I understand it is desired to get this out, I think it is still in a position where it isn't easily maintainable and useable long term, which is what a WebExtension API is meant to be.

Can we discuss and maybe make a few more changes for usability please? (Side question: Should Thunderbird in general have some sort of community review/feedback process for WebExtension APIs for extension developer feedback? I don't know what Firefox does here).

For example, If I want to get an item from the address book, and I have its id, I feel I should just be able to do: browser.addressBooks.get(id);

At the moment, I have to worry about if it is an address book, contact or mailingList, and then find the right API to call.

I think the create/update/delete functions could also be handled similarly, with the function itself working out based on the type passed to it, e.g. browser.addressBooks.create({ type: addressBook, name: "foo"});

Some other comments:

- For creating/updating an address book itself, we shouldn't be passing just a string. That prevents the possibility for future fields without awkward API (e.g. what about LDAP & other types).
- iirc some address books (e.g. Mac, LDAP), can be read-only. It isn't clear to me how that is addressed/handled.
- It might be useful for the listener functions to also pass the newly added/updated object so that there doesn't have to be a subsequent api/database access by a WebExtension to get the new details.

Comment 22

[Geoff Lankow (:darktrojan)]

(In reply to Mark Banner (:standard8) from comment #21)
> For example, If I want to get an item from the address book, and I have its
> id, I feel I should just be able to do: browser.addressBooks.get(id);
> 
> At the moment, I have to worry about if it is an address book, contact or
> mailingList, and then find the right API to call.

Why would you have something's ID without knowing what sort of object it is? And what are you going to do with it, if you don't know what sort of object it is?

> - For creating/updating an address book itself, we shouldn't be passing just
> a string. That prevents the possibility for future fields without awkward
> API (e.g. what about LDAP & other types).

I can see where you're coming from here, in fact I think I might change this to an object, but if we make future changes we'd still have to change the API and extensions would have to adapt, as the mechanism enforces the names and types that can be passed around.

> - iirc some address books (e.g. Mac, LDAP), can be read-only. It isn't clear
> to me how that is addressed/handled.

If a book is read-only it should enforce that with exceptions. Exceptions are handled by the WebExtensions mechanism, which return a failure to the calling code.

> - It might be useful for the listener functions to also pass the newly
> added/updated object so that there doesn't have to be a subsequent
> api/database access by a WebExtension to get the new details.

I did want to do this, but to do so would involve rebuilding the cache at every event (mildly expensive), and in many cases multiple events happen at once. I'd rather have the cost only if and when the new information is wanted.

Comment 23

[Geoff Lankow (:darktrojan)]

Attachment: 1469238-webext-addressbook-7.diff

Yes, I like that better with object arguments. We can add further optional arguments without breaking extensions.

Comment 24

[Mark Banner (:standard8)]

(In reply to Geoff Lankow (:darktrojan) from comment #22)
> (In reply to Mark Banner (:standard8) from comment #21)
> > For example, If I want to get an item from the address book, and I have its
> > id, I feel I should just be able to do: browser.addressBooks.get(id);
> > 
> > At the moment, I have to worry about if it is an address book, contact or
> > mailingList, and then find the right API to call.
> 
> Why would you have something's ID without knowing what sort of object it is?
> And what are you going to do with it, if you don't know what sort of object
> it is?

So some of these thoughts are based on how the Bookmarks API works in Firefox: https://developer.mozilla.org/docs/Mozilla/Add-ons/WebExtensions/API/bookmarks - note, there's three different types of bookmarks there as well (bookmark, separator, folder).

I think one obvious case is delete. If my extension displayed a list of items from address books, and let the user to select them and then choose them to delete, with the current API I'd have to do something like:

for (let id of ids) {
  select(bm[id].type) {
    case "mailingList":
      browser.mailingList.delete(id);
      break;
    case "contact":
      browser.contacts.delete(id);
      break;
    }
  }
}

Whereas with a single API, I'd be able to do:

for (let id of ids) {
  browser.addressBooks.delete(id);
}

and be done with it. In the first case, if a new type got added, I'd need to go back and adjust that loop again.

(In reply to Geoff Lankow (:darktrojan) from comment #22)
> > - iirc some address books (e.g. Mac, LDAP), can be read-only. It isn't clear
> > to me how that is addressed/handled.
> 
> If a book is read-only it should enforce that with exceptions. Exceptions
> are handled by the WebExtensions mechanism, which return a failure to the
> calling code.

So that wasn't clear from the documentation, but maybe that's a limitation of the json format. I also didn't see any handling within the API code itself, which makes me wonder what exceptions would get passed back.

> > - It might be useful for the listener functions to also pass the newly
> > added/updated object so that there doesn't have to be a subsequent
> > api/database access by a WebExtension to get the new details.
> 
> I did want to do this, but to do so would involve rebuilding the cache at
> every event (mildly expensive), and in many cases multiple events happen at
> once. I'd rather have the cost only if and when the new information is
> wanted.

Can't you just rebuild the specific item within the cache? Or just update it from the notification?

I could go either way. The only other thing to consider is if you could get to the stage where with multiple extensions installed they all want to get the updated object after an update, then you'd be multiplying up requests.

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 9005104 [details] [diff] [review]
1469238-webext-addressbook-7.diff

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

[more comments later]

::: mail/components/extensions/parent/ext-addressBook.js
@@ +5,5 @@
> +Cu.importGlobalProperties(["btoa", "crypto"]);
> +
> +ChromeUtils.import("resource://gre/modules/Services.jsm");
> +const { fixIterator } = ChromeUtils.import("resource:///modules/iteratorUtils.jsm", {});
> +const { MailServices } = ChromeUtils.import("resource:///modules/mailServices.js", {});

{} or null? 
Someone commented on this in a bug (I think you were involved), but I don't recall the outcome on which was preferred.

@@ +93,5 @@
> +    this._tree = null;
> +  }
> +  generateId() {
> +    const ID_LENGTH = 12;
> +    return btoa(String.fromCharCode(...crypto.getRandomValues(new Uint8Array(ID_LENGTH / 4 * 3))));

I'd still prefer uuid v4. That's just common practice for uniqueness, and the APIs are supposed to cater for use cases ranging outside local usage.
Just look at vcard handling, they use UIDs too, globally unique ones. Just handling those would require some mappings, which gets quite silly.

@@ +190,5 @@
> +        this.emit("book_created", item);
> +        this._addressBookIds.push(item.UID);
> +      }
> +    } catch (ex) {
> +    }

what's the catches for?

@@ +251,5 @@
> +  }
> +};
> +MailServices.ab.addAddressBookListener(cache, Ci.nsIAbListener.all);
> +
> +

nit: extra blank line

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 9005104 [details] [diff] [review]
1469238-webext-addressbook-7.diff

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

::: mail/components/extensions/parent/ext-addressBook.js
@@ +442,5 @@
> +          mailList.dirName = name;
> +          mailList.listNickName = nickName === null ? "" : nickName;
> +          mailList.description = description === null ? "" : description;
> +
> +          let parentNode = cache.findObjectById("addressBook", parentId);

need error handling for then the parentId is (no longer) found
that's probably needed elsewhere too

Comment 27

[Geoff Lankow (:darktrojan)]

Attachment: 1469238-webext-addressbook-8.diff

Comment 28

[Geoff Lankow (:darktrojan)]

I haven't mentioned it yet, but I'm working on another patch to convert the get, delete, and add/removeMember methods to accept an array of inputs. This will make the output of get methods an array also, but otherwise should not affect things. I hope to have them landing at the same time anyway.

Comment 29

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Mark Banner (:standard8) from comment #21)
> Can we discuss and maybe make a few more changes for usability please? (Side
> question: Should Thunderbird in general have some sort of community
> review/feedback process for WebExtension APIs for extension developer
> feedback? I don't know what Firefox does here).

Firefox has a regular triage meeting to decide on if an API should be accepted, in which maybe some initial implementation details are discussed. The remaining discussion mostly happens on the bug, or I guess internally with the engineers working on it. The sidebar bug 1208596 is an example of a new API implemented.

Another thing we could do is start this as an experiment API on https://github.com/thundernest/tb-web-ext-experiments and identify add-ons that could use this API. Get in touch with the developers of those add-ons and ask them if they could give feedback, or try to implement their add-on with the API using either a combined experiment/webextension, or a Legacy Embedded Extension.

Comment 30

[Magnus Melin [:mkmelin]]

Comment on attachment 9005537 [details] [diff] [review]
1469238-webext-addressbook-8.diff

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

From a life-cycle perspective, we seem to be missing the pre-creation/alteration/deletion hooks. An extension might want to e.g. switch the firstname/lastname around if it's wrong.

::: mail/components/extensions/parent/ext-addressBook.js
@@ +92,5 @@
> +  flush() {
> +    this._tree = null;
> +  }
> +  generateId() {
> +    return uuidGenerator.generateUUID().number.substring(1, 37);

you could generate it yourself too, comment 7. should be faster, but that's not so much of a concern

@@ +94,5 @@
> +  }
> +  generateId() {
> +    return uuidGenerator.generateUUID().number.substring(1, 37);
> +  }
> +  findObjectById(type, id) {

maybe it's still better to make this _findObjectById and "expose" findAddressBookById, findMailingListById and findContactById

@@ +128,5 @@
> +      "addressBook": "Address book",
> +      "contact": "Contact",
> +      "mailingList": "Mailing list"
> +    }[type];
> +    throw new ExtensionError(`${verboseType} "${id}" could not be found.`);

I don't understand the verboseType thing here. Wouldn't that always be undefined?


But more importantly. Shouldn't a not-found just return null? It's not an error per se. It's just that the caller must handle it gracefully

@@ +184,5 @@
> +
> +    if (item instanceof Ci.nsIAbDirectory) {
> +      item = item.QueryInterface(Ci.nsIAbDirectory);
> +      if (!item.UID) {
> +        item.UID = this.generateId();

hmm, getting/setting an id only after it was added seems odd.

More in general, we need to think about who is responsible for ensuring everything has an id, even if a web extension didn't create it.

@@ +222,5 @@
> +            this._addressBookIds.splice(i, 1);
> +            break;
> +          }
> +        }
> +        this.emit("book_deleted", deletedId);

I'm not sure I understand what the deletedId is about. Wouldn't it always be the item.uid?

@@ +261,5 @@
> +          topWindow.focus();
> +        },
> +        closeUI() {
> +          var windowEnum = Services.wm.getEnumerator(AB_WINDOW_TYPE);
> +          while (windowEnum.hasMoreElements()) {

for (let win of Services.wm.getEnumerator(AB_WINDOW_TYPE)) {

@@ +359,5 @@
> +            if (hiddenProperties.includes(property.name)) {
> +              continue;
> +            }
> +            if (!(property.name in properties)) {
> +              node.item.setProperty(property.name, null);

so this makes is a deep update?

maybe we should make it a shallow one instead, so that only specified props are changed?

::: mail/components/extensions/schemas/addressBook.json
@@ +290,5 @@
> +    "functions": [
> +      {
> +        "name": "list",
> +        "type": "function",
> +        "async": "callback",

what are the alternatives?

Comment 31

[Magnus Melin [:mkmelin]]

Comment on attachment 9005537 [details] [diff] [review]
1469238-webext-addressbook-8.diff

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

::: mail/components/extensions/parent/ext-addressBook.js
@@ +56,5 @@
> +        } else {
> +          this.contacts = [...fixIterator(directory.childCards, Ci.nsIAbCard)]
> +            .filter(c => !c.isMailList);
> +        }
> +        this.contacts = this.contacts.map(c => cache._makeContactNode(c, directory));

This doesn't look like it would play out very well with large LDAP address books and similar setups.
In fact any contact caching is problematic in such cases.

@@ +233,5 @@
> +    }
> +
> +    this._tree = null;
> +  }
> +  onItemPropertyChanged(item, property, oldValue, newValue) {

I realize this is coming from the current implementation. But in some ways it is sub-optimal: quite likely more than one property changed at once. Not sure why the current implementation does it this way (perhaps because in cpp it would be awkward otherwise), but callers aren't going to be that interested in individual property updates. 

Compare to componentDidUpdate(prevProps, prevState) in React.

@@ +369,5 @@
> +            }
> +          }
> +          parentNode.item.modifyCard(node.item);
> +        },
> +        delete(id) {

Did we expose the addressbook readonly state? If not, delete may well fail.
As for error handling, I suggest the delete method returns the id if the contact was found and deleted. Otherwise null.

@@ +458,5 @@
> +          let node = cache.findObjectById("mailingList", id);
> +          let contactNode = cache.findObjectById("contact", contactId);
> +          node.item.addCard(contactNode.item);
> +        },
> +        listMembers(id) {

perhaps getMembers is a more consistent naming

Comment 32

[Geoff Lankow (:darktrojan)]

(In reply to Magnus Melin from comment #30)
> From a life-cycle perspective, we seem to be missing the
> pre-creation/alteration/deletion hooks. An extension might want to e.g.
> switch the firstname/lastname around if it's wrong.

Possibly, but that can be added later if we wanted it, especially as it'd be a large invasive change on the backend.

> > +  generateId() {
> > +    return uuidGenerator.generateUUID().number.substring(1, 37);
> 
> you could generate it yourself too, comment 7. should be faster, but that's
> not so much of a concern

We've got a component to do this for us, why would I reimplement it myself?

> @@ +128,5 @@
> > +      "addressBook": "Address book",
> > +      "contact": "Contact",
> > +      "mailingList": "Mailing list"
> > +    }[type];
> > +    throw new ExtensionError(`${verboseType} "${id}" could not be found.`);
> 
> I don't understand the verboseType thing here. Wouldn't that always be
> undefined?

type is always one of addressBook, contact, and mailingList. Why would it be undefined?

> But more importantly. Shouldn't a not-found just return null? It's not an
> error per se. It's just that the caller must handle it gracefully

In all the cases I've currently got, this should throw to be consistent with other WebExtension APIs. (Don't ask why, I didn't design them.) I can think of cases where it shouldn't throw, but we don't have any of them at the moment.

> > +    if (item instanceof Ci.nsIAbDirectory) {
> > +      item = item.QueryInterface(Ci.nsIAbDirectory);
> > +      if (!item.UID) {
> > +        item.UID = this.generateId();
> 
> hmm, getting/setting an id only after it was added seems odd.
> 
> More in general, we need to think about who is responsible for ensuring
> everything has an id, even if a web extension didn't create it.

If we're getting it through the web extension, it has an ID. They get added in _makeContactNode and _makeDirectoryNode if they don't exist already.

> > +        this.emit("book_deleted", deletedId);
> 
> I'm not sure I understand what the deletedId is about. Wouldn't it always be
> the item.uid?

It would be, but for some reason I don't quite understand, it isn't there any more.

> @@ +359,5 @@
> > +            if (hiddenProperties.includes(property.name)) {
> > +              continue;
> > +            }
> > +            if (!(property.name in properties)) {
> > +              node.item.setProperty(property.name, null);
> 
> so this makes is a deep update?
> 
> maybe we should make it a shallow one instead, so that only specified props
> are changed?

You told me to do that in comment 17. But I'd rather it was a shallow update now that I know how to change the schema so it behaves differently.

> ::: mail/components/extensions/schemas/addressBook.json
> @@ +290,5 @@
> > +    "functions": [
> > +      {
> > +        "name": "list",
> > +        "type": "function",
> > +        "async": "callback",
> 
> what are the alternatives?

I don't understand the question.

Comment 33

[Geoff Lankow (:darktrojan)]

(In reply to Magnus Melin from comment #31)
> > +        } else {
> > +          this.contacts = [...fixIterator(directory.childCards, Ci.nsIAbCard)]
> > +            .filter(c => !c.isMailList);
> > +        }
> > +        this.contacts = this.contacts.map(c => cache._makeContactNode(c, directory));
> 
> This doesn't look like it would play out very well with large LDAP address
> books and similar setups.
> In fact any contact caching is problematic in such cases.

Yeah. Make it an iterator/generator?

> > +  onItemPropertyChanged(item, property, oldValue, newValue) {
> 
> I realize this is coming from the current implementation. But in some ways
> it is sub-optimal: quite likely more than one property changed at once. Not
> sure why the current implementation does it this way (perhaps because in cpp
> it would be awkward otherwise), but callers aren't going to be that
> interested in individual property updates. 
> 
> Compare to componentDidUpdate(prevProps, prevState) in React.

You're talking about when a contact is updated and lots of properties change? In fact this only fires once in that case.
onItemPropertyChanged is truly awful. Only the item argument is remotely reliable.

> 
> @@ +369,5 @@
> > +            }
> > +          }
> > +          parentNode.item.modifyCard(node.item);
> > +        },
> > +        delete(id) {
> 
> Did we expose the addressbook readonly state?

No, but we could.

> If not, delete may well fail.
> As for error handling, I suggest the delete method returns the id if the
> contact was found and deleted. Otherwise null.

Failure will result in an exception which the WE framework will handle.
 
> @@ +458,5 @@
> > +          let node = cache.findObjectById("mailingList", id);
> > +          let contactNode = cache.findObjectById("contact", contactId);
> > +          node.item.addCard(contactNode.item);
> > +        },
> > +        listMembers(id) {
> 
> perhaps getMembers is a more consistent naming

I've used "list" everywhere that returns all items and "get" to return specified item(s).

Comment 34

[Magnus Melin [:mkmelin]]

(In reply to Geoff Lankow (:darktrojan) from comment #32)
> We've got a component to do this for us, why would I reimplement it myself?

I'll leave it up to you if you want to use it or not. 
Why? Speed and avoiding xpcom if a replacement is only a few lines.

> > @@ +128,5 @@
> > > +      "addressBook": "Address book",
> > > +      "contact": "Contact",
> > > +      "mailingList": "Mailing list"
> > > +    }[type];
> > > +    throw new ExtensionError(`${verboseType} "${id}" could not be found.`);
> > 
> > I don't understand the verboseType thing here. Wouldn't that always be
> > undefined?
> 
> type is always one of addressBook, contact, and mailingList. Why would it be
> undefined?

Well, if someone mistypes the type? I would just put the raw ${type} in the message.

> > But more importantly. Shouldn't a not-found just return null? It's not an
> > error per se. It's just that the caller must handle it gracefully
> 
> In all the cases I've currently got, this should throw to be consistent with
> other WebExtension APIs. (Don't ask why, I didn't design them.) I can think
> of cases where it shouldn't throw, but we don't have any of them at the
> moment.

Any pointers to those other apis?
Seems like bad design to me. It's a perfectly valid use case to look up to see if an contact exists, for instance. 

> > > +    if (item instanceof Ci.nsIAbDirectory) {
> > > +      item = item.QueryInterface(Ci.nsIAbDirectory);
> > > +      if (!item.UID) {
> > > +        item.UID = this.generateId();
> > 
> > hmm, getting/setting an id only after it was added seems odd.
> > 
> > More in general, we need to think about who is responsible for ensuring
> > everything has an id, even if a web extension didn't create it.
> 
> If we're getting it through the web extension, it has an ID. They get added
> in _makeContactNode and _makeDirectoryNode if they don't exist already.

Yes internally. Maybe it would make sense not to tie that into wx, but if not available, set it up in ::GetUID methods?
I don't want to end up with a situation that some abs have ids and others don't, depending on whether wx usage was involved.

> > > +        this.emit("book_deleted", deletedId);
> > 
> > I'm not sure I understand what the deletedId is about. Wouldn't it always be
> > the item.uid?
> 
> It would be, but for some reason I don't quite understand, it isn't there
> any more.

I'd suggest debugging that.

> > @@ +359,5 @@
> > > +            if (hiddenProperties.includes(property.name)) {
> > > +              continue;
> > > +            }
> > > +            if (!(property.name in properties)) {
> > > +              node.item.setProperty(property.name, null);
> > 
> > so this makes is a deep update?
> > 
> > maybe we should make it a shallow one instead, so that only specified props
> > are changed?
> 
> You told me to do that in comment 17. But I'd rather it was a shallow update
> now that I know how to change the schema so it behaves differently.

I don't see that, anyway, I'm glad we agree :)

> > ::: mail/components/extensions/schemas/addressBook.json
> > @@ +290,5 @@
> > > +    "functions": [
> > > +      {
> > > +        "name": "list",
> > > +        "type": "function",
> > > +        "async": "callback",
> > 
> > what are the alternatives?
> 
> I don't understand the question.

What does "async": "callback" implicate? That the function is marked async? Are there other async versions, etc.?

Comment 35

[Magnus Melin [:mkmelin]]

(In reply to Geoff Lankow (:darktrojan) from comment #33)
> (In reply to Magnus Melin from comment #31)
> > > +        } else {
> > > +          this.contacts = [...fixIterator(directory.childCards, Ci.nsIAbCard)]
> > > +            .filter(c => !c.isMailList);
> > > +        }
> > > +        this.contacts = this.contacts.map(c => cache._makeContactNode(c, directory));
> > 
> > This doesn't look like it would play out very well with large LDAP address
> > books and similar setups.
> > In fact any contact caching is problematic in such cases.
> 
> Yeah. Make it an iterator/generator?

With large LDAP you're not likely to ever get/want the complete list into memory.
So I'm not sure what the solution is. You may have to look up on the go, dropping the cache.

> > > +  onItemPropertyChanged(item, property, oldValue, newValue) {
> > 
> > I realize this is coming from the current implementation. But in some ways
> > it is sub-optimal: quite likely more than one property changed at once. Not
> > sure why the current implementation does it this way (perhaps because in cpp
> > it would be awkward otherwise), but callers aren't going to be that
> > interested in individual property updates. 
> > 
> > Compare to componentDidUpdate(prevProps, prevState) in React.
> 
> You're talking about when a contact is updated and lots of properties
> change? In fact this only fires once in that case.
> onItemPropertyChanged is truly awful. Only the item argument is remotely
> reliable.

Ok, let's not expose something that is telling lies. Ideally prev + new states, but if not available, maybe just onItemPropertyChanged(item)?

> > Did we expose the addressbook readonly state?
> 
> No, but we could.

Let's do that then.

> 
> > If not, delete may well fail.
> > As for error handling, I suggest the delete method returns the id if the
> > contact was found and deleted. Otherwise null.
> 
> Failure will result in an exception which the WE framework will handle.

This all depends on what we want to expose. Yes xpcom would throw if something went wrong. But what if we envision not using xpcom for storage. Then deleting something non-existent is not really something you'd throw for (e.g. a database call will be happy to delete a non-existent row without errors.)

> > @@ +458,5 @@
> > > +          let node = cache.findObjectById("mailingList", id);
> > > +          let contactNode = cache.findObjectById("contact", contactId);
> > > +          node.item.addCard(contactNode.item);
> > > +        },
> > > +        listMembers(id) {
> > 
> > perhaps getMembers is a more consistent naming
> 
> I've used "list" everywhere that returns all items and "get" to return
> specified item(s).

Ok

Comment 36

[Geoff Lankow (:darktrojan)]

Attachment: 1469238-webext-addressbook-9.diff

Some significant changes here:

* Read-only address books are ignored for the time being. That's not ideal but I want to concentrate on getting the core of the API ready first.
* I'm now listening for some observer notifications (bug 1490626) so that we have exactly one event where we should have one event.
* onCreated and onUpdated events now return a node, not an id.
* Callbacks are removed from the schema, as it turns out the way I was doing it was just a hangover from supporting Chrome WebExtensions.
* The test now checks that exceptions are thrown when trying to operate on items that don't exist.
* The test now checks that events fire when modifications are made from outside WebExtensions.

Comment 37

[Geoff Lankow (:darktrojan)]

(In reply to Magnus Melin from comment #34)
> > type is always one of addressBook, contact, and mailingList. Why would it be
> > undefined?
> 
> Well, if someone mistypes the type? I would just put the raw ${type} in the
> message.

This is internal code that can't be reached unless you're making a change to comm-central. If you mistype it there, it's on you.

> > In all the cases I've currently got, this should throw to be consistent with
> > other WebExtension APIs. (Don't ask why, I didn't design them.) I can think
> > of cases where it shouldn't throw, but we don't have any of them at the
> > moment.
> 
> Any pointers to those other apis?
> Seems like bad design to me. It's a perfectly valid use case to look up to
> see if an contact exists, for instance. 

bookmarks.get: "If no nodes could be found, the promise will be rejected with an error message."
bookmarks.remove: "If the node corresponding to the id parameter can't be found … the promise is rejected with an error message."
contextualIdentities.remove: "If the identity could not be found or the contextual identities feature is not enabled, the promise is rejected."
tabs.get: "If the tab could not be found or some other error occurs, the promise will be rejected with an error message."

I actually agree with you, this is a stupid pattern, but I think it's better to be consistent.

> > > I'm not sure I understand what the deletedId is about. Wouldn't it always be
> > > the item.uid?
> > 
> > It would be, but for some reason I don't quite understand, it isn't there
> > any more.
> 
> I'd suggest debugging that.

The notification occurs after the address book has been removed, and the info comes from prefs, which are gone by the time we get to the notification… 
… waaait a minute, I can fix that.

> > > ::: mail/components/extensions/schemas/addressBook.json
> > > @@ +290,5 @@
> > > > +    "functions": [
> > > > +      {
> > > > +        "name": "list",
> > > > +        "type": "function",
> > > > +        "async": "callback",
> > > 
> What does "async": "callback" implicate? That the function is marked async?
> Are there other async versions, etc.?

It turns out, by reading the code instead of the documentation, that "callback" refers to the parameter I named callback, and that I don't need it. I can use "async": true to get the desired result.

(In reply to Magnus Melin from comment #35)
> > > > +  onItemPropertyChanged(item, property, oldValue, newValue) {
> > > 
> > > I realize this is coming from the current implementation. But in some ways
> > > it is sub-optimal: quite likely more than one property changed at once. Not
> > > sure why the current implementation does it this way (perhaps because in cpp
> > > it would be awkward otherwise), but callers aren't going to be that
> > > interested in individual property updates. 
> > > 
> > > Compare to componentDidUpdate(prevProps, prevState) in React.
> > 
> > You're talking about when a contact is updated and lots of properties
> > change? In fact this only fires once in that case.
> > onItemPropertyChanged is truly awful. Only the item argument is remotely
> > reliable.
> 
> Ok, let's not expose something that is telling lies. Ideally prev + new
> states, but if not available, maybe just onItemPropertyChanged(item)?

We're not, this isn't exposed anywhere.

Comment 38

[Geoff Lankow (:darktrojan)]

Attachment: 1469238-webext-addressbook-10.diff

Now without special handling for deleted address books.

Comment 39

[Geoff Lankow (:darktrojan)]

With the patches for this bug and the ones that block it, (as yet incomplete) sample extension runs:
https://github.com/darktrojan/sample-extensions/tree/master/addressBooks

Comment 40

[Magnus Melin [:mkmelin]]

That wx sample is a good start. I think it would be useful to be able to add contacts to lists, and lists to address books, and such so more real functionality can be tried out there. (I couldn't create a contact yet, only lists and address books.)

BTW, mind switching to 2 space indention?

Also, get this in the console:

Reading manifest: Error processing browser_action: An unexpected property was found in the WebExtension manifest.

Comment 41

[Magnus Melin [:mkmelin]]

Comment on attachment 9008980 [details] [diff] [review]
1469238-webext-addressbook-10.diff

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

Some more random feedback skimming this through

::: mail/components/extensions/parent/ext-addressBook.js
@@ +13,5 @@
> +const kPABDirectory = 2; // defined in nsDirPrefs.h
> +
> +// nsIAbCard.idl contains a list of properties that Thunderbird uses. Extensions are not
> +// restricted to using only these properties, but the following properties cannot
> +// be used by an extension.

be modified by and extension

@@ +114,5 @@
> +      addressBook: "Address book",
> +      contact: "Contact",
> +      mailingList: "Mailing list",
> +    }[type];
> +    throw new ExtensionError(`${verboseType} "${id}" could not be found.`);

I'd still just put the ${type} in there. "${type} with id=${id} could not be found."

@@ +173,5 @@
> +
> +    return copy;
> +  }
> +
> +  // nsIAbListener

Please make it a documentation comment. E.g. with triple slash

/// @implements {nsIAbListener}


and add that kind of info to the others as well

@@ +182,5 @@
> +      item.QueryInterface(Ci.nsIAbDirectory);
> +      if (item.isMailList) {
> +        this.emit("list_created", this._makeDirectoryNode(item, parent));
> +      } else {
> +        this.emit("book_created", this._makeDirectoryNode(item));

maybe addrbook-created instead.

Looks like convention is to use - instead of _ in the emitted names

Comment 42

[[e-mail]]

I've been trying to write a WebExtension that maps Thunderbird contacts to a third-party web-based remote contact store.

Although it would be technically possible to use the `jsaccount` mechanism to create my own directory type, I thought it would be simpler for me just to try to synchronise the third-party store with a local address book using an extension API similar to the one here, which I've also been writing as I go in order to determine the feasibility of the approach.

Some problems I've run into so far are as follows:

* I need to link the remote store to a specific address book. My current "solution" involves using the addressbook creation API that takes a directoryURI as a parameter, which allows the addressbook to be easily found later. Perhaps you can suggest a better solution?

* I would like to be able to relate remote address lists to local address lists. Unfortunately I can't store anything on an address list object that I can retrieve later to know that it's one I stored earlier. However, seeing as that I don't know what changes might have been made on the remote service while Thunderbird is closed, I might just go with deleting and recreating all address lists on each startup, and then keeping an in-memory list of lists, so to speak, which I can then use to update the remote store when changes are made locally.

Comment 43

[[e-mail]]

Comment on attachment 9008980 [details] [diff] [review]
1469238-webext-addressbook-10.diff

>+      case "addrbook-list-updated": {
>+        this.emit("list_updated", this.findMailingListById(subject.UID));
Don't you need to QI `subject` first?

Comment 44

[[e-mail]]

Comment on attachment 9008980 [details] [diff] [review]
1469238-webext-addressbook-10.diff

>+        "description": "Edits the properties of a contact. Properties not specified in <code>properties</code> will be removed if they existed."
I thought attempting to set the property to the empty string deleted it. This way makes it harder to modify selected properties.

Comment 45

[Geoff Lankow (:darktrojan)]

Hi Neil, that documentation is out of date, and I've now fixed it. To remove a property, set it to null.

Comment 46

[Magnus Melin [:mkmelin]]

(In reply to [e-mail] from comment #42)
> I've been trying to write a WebExtension that maps Thunderbird contacts to a
> third-party web-based remote contact store.
> 
> Although it would be technically possible to use the `jsaccount` mechanism
> to create my own directory type, I thought it would be simpler for me just
> to try to synchronise the third-party store with a local address book using
> an extension API similar to the one here, which I've also been writing as I
> go in order to determine the feasibility of the approach.
> 
> Some problems I've run into so far are as follows:
> 
> * I need to link the remote store to a specific address book. My current
> "solution" involves using the addressbook creation API that takes a
> directoryURI as a parameter, which allows the addressbook to be easily found
> later. Perhaps you can suggest a better solution?

Creation is 

  create({ name }) 

So you give in a name to create it. create return the UID of the newly created ab, and that's what you'd use to find it later.

> * I would like to be able to relate remote address lists to local address
> lists. Unfortunately I can't store anything on an address list object that I
> can retrieve later to know that it's one I stored earlier. However, seeing
> as that I don't know what changes might have been made on the remote service
> while Thunderbird is closed, I might just go with deleting and recreating
> all address lists on each startup, and then keeping an in-memory list of
> lists, so to speak, which I can then use to update the remote store when
> changes are made locally.

I suppose there should be a way to find items changed after a given timestamp, so that if they have changed you know to sync them.

Re "to know that it's one I stored earlier" - everything has a UID now so if you keep track of the UID you know it's the same item.

Comment 47

[[e-mail]]

(In reply to Magnus Melin from comment #46)
> Re "to know that it's one I stored earlier" - everything has a UID now so if
> you keep track of the UID you know it's the same item.

Yes but basically that means I now have to maintain an internal store which knows the Thunderbird UID and remote store's ID for every contact and reconcile the internal store with both Thunderbird and the remote store...

Comment 48

[Magnus Melin [:mkmelin]]

Right, sounds like we should consider allowing setting the UID for contacts if they have one "in the wild" already.

Comment 49

[Mark Banner (:standard8)]

(In reply to Magnus Melin [:mkmelin] from comment #48)
> Right, sounds like we should consider allowing setting the UID for contacts
> if they have one "in the wild" already.

How would that work for multiple extensions that all had their own UIDs? or if a UID was already set?

I'd be wary of recording extra information in the database apart from what Thunderbird itself requires - for example, if the format ever changes, you would then need to work out how to migrate it all and there may be data you don't know about. WebExtensions have their own store, so they can easily record extra information (this is also the general principle of how Firefox's bookmarks work wrt WebExtensions, any extra information is the responsibility of the add-on to maintain).

Comment 50

[Magnus Melin [:mkmelin]]

Sorry to be unclear, I meant allow setting the UID *during the creation*. After the contact is created I agree it can't be changed.

If you allow setting UID during creation that means you can have an outside contact source which is the authoritative source of information. I assume this is what Neil wanted to achieve.

Comment 51

[Geoff Lankow (:darktrojan)]

Attachment: 1469238-webext-addressbook-11.diff

Comment 52

[Magnus Melin [:mkmelin]]

Comment on attachment 9011713 [details] [diff] [review]
1469238-webext-addressbook-11.diff

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

As discussed there are still improvements and changes to be done, but let's land this and do those in separate bugs. r=mkmline

::: mail/components/extensions/parent/ext-addressBook.js
@@ +18,5 @@
> +const hiddenProperties = [
> +  "DbRowID", "LowercasePrimaryEmail", "LastModifiedDate",
> +  "PopularityIndex", "RecordKey", "UID",
> +];
> +

/**
 * Cache + add some text
 * @implements {nsIAbListener}
 * @implements {nsIObserver}
 */

@@ +168,5 @@
> +
> +    return copy;
> +  }
> +
> +  // nsIAbListener

you could add this for the other implemented methods too for clarity

@@ +242,5 @@
> +        let parentNode = this.findMailingListById(data);
> +        this.emit("mailing-list-member-added", this._makeContactNode(subject, parentNode.item));
> +        break;
> +      }
> +      default:

braces here too

@@ +259,5 @@
> +  getAPI(context) {
> +    return {
> +      addressBooks: {
> +        openUI() {
> +          var topWindow = Services.wm.getMostRecentWindow(AB_WINDOW_TYPE);

let

@@ +432,5 @@
> +          mailList.isMailList = true;
> +          mailList.dirName = name;
> +          mailList.listNickName = nickName === null ? "" : nickName;
> +          mailList.description = description === null ? "" : description;
> +

I'd put () around the expressions like (nickName === null)

here and in a few other places

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4dbfa038c936
Address Book WebExtensions API; r=mkmelin

Comment 54

[Geoff Lankow (:darktrojan)]

Backed out again while I figure out
1. why the schema check only seems to run in debug mode which means I need a debug build to see it
2. what's actually wrong with the schema.

Grrr!

Comment 55

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/aa44e834870f
Backed out changeset 4dbfa038c936 for causing test failures

Comment 56

[Geoff Lankow (:darktrojan)]

Attachment: 1469238-webext-addressbook-12.diff

Fixed the faulty schema (still don't know why this is only checked on debug builds), and the shutdown crash that happened after that.

Comment 57

[Jorg K (CEST = GMT+2)]

Next step landing again?

Comment 58

[Jorg K (CEST = GMT+2)]

I did another "secret" try run with this
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=201953183&revision=2a8bbf89bce53e048997d1e01c55cf3d69836c9d
and test_ext_addressBook.js timed out on Mac on both opt and debug. Bad luck?

Comment 59

[Geoff Lankow (:darktrojan)]

No, something's wrong, probably to do with the Mac address book. This is going to be "fun" to figure out.

Comment 60

[Jorg K (CEST = GMT+2)]

Silly question, can the Mac address book integration be turned off during the test?

Comment 61

[Geoff Lankow (:darktrojan)]

I don't think it can without major changes, but in any case I'm concerned that existing stuff is being broken on Mac.

It would be helpful if the test logs would give me any information at all. It seems like it is dying without even an exception.

Comment 62

[Geoff Lankow (:darktrojan)]

Okay, even without my changes from bug 1482040 and bug 1490626, the really basic test* fails on OS X (only) in the same way. No output after the first line, no exceptions, just times out. I think this means I haven't broken anything, which is good, but what on Earth is wrong with my test?

* https://hg.mozilla.org/try-comm-central/file/0510412cd4ff/mail/components/extensions/test/xpcshell/test_ext_addressBook.js

Comment 63

[Geoff Lankow (:darktrojan)]

Late-night brainwave: it turns out I *can* disable OSX-only stuff, and that happens in the mailnews tests, which is why running (essentially) the same code there works. Try run in progress.

Comment 64

[Geoff Lankow (:darktrojan)]

Attachment: 1469238-webext-addressbook-13.diff

Yes, that works. Finally.

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1bc7b3e7b9db
Address Book WebExtensions API; r=mkmelin

Comment 66

[Magnus Melin [:mkmelin]]

Please file follow-ups if you have improvement suggestions.

Comment 67

[[e-mail]]

(In reply to comment #42)
> * I need to link the remote store to a specific address book. My current
> "solution" involves using the addressbook creation API that takes a
> directoryURI as a parameter, which allows the addressbook to be easily found
> later. Perhaps you can suggest a better solution?

So, it turns out that using the uuid of the addressbook is flawed, because renaming the address book changes its uuid anyway...

Comment 68

[Ben Bucksch (:BenB)]

@Neil: Can you file a bug for that? The UUID should stay the same, that's the whole point, otherwise you could just as well use the name as ID.

Comment 69

[[e-mail]]

(In reply to Ben Bucksch from comment #68)

The UUID should stay the same, that's the whole point, otherwise you could just as well use the name as ID.

My bad, I was confusing the UID (stays the same) with the uuid (obsolete, changes).

Comment 70

[Ben Bucksch (:BenB)]

Easy to confuse indeed :)

Comment 71

[[e-mail]]

Comment on attachment 9013461 [details] [diff] [review]
1469238-webext-addressbook-13.diff

>+        this.emit("contact-created", this._makeContactNode(subject, parentNode.item));
If the extension throws an exception, then I think that ends up in a Promise, which gets ignored here. Are extensions expected to handle their own exceptions in their listeners, and if so, in what way, just console logging, or something else?

Comment 72

[Ben Bucksch (:BenB)]

I filed bug 1597236 about that.