841603 - Create a generic ContactService to sit in front of the current address book

Status: RESOLVED WONTFIX

(MailNews Core :: Address Book, defect)

Description

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

MailNews uses nsIAbManager to interface with the current address book, but that interface is a little problematic:

1) It is synchronous
2) It exposes too much about the underlying architecture of the address book.

What I'd like to do is put a minimal interface between Mailnews and the address book, providing both asynchronous and synchronous (marked deprecated) methods.

Then, I'd like to implement the interface for the current address book, implementing both the synchronous and asynchronous methods.

Then, I'll alter mailnews/ (and mail/) to use the new synchronous methods for the new interface. If I'm lucky, I'll find some places where I can switch to asynchronous method calls, and I'll use those instead.

After that, call by call, I'll start modifying MailNews to use the asynchronous methods.

That's my plan, anyhow.

Comment 1

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

Attachment: WIP Patch 0.1

Checkpointing some boilerplate.

Comment 2

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

Attachment: WIP patch 0.2

Moving the nsIAbCard converter over from Ensemble, created a new mozIContact interface that is composed of nsIDOMContact along with some extra fields that we need.

Comment 3

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

Attachment: WIP patch 0.2

Whoops - included the wrong IDL version.

Comment 4

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

Attachment: WIP patch 0.3

Comment 5

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

Attachment: Patch v1

This is a base patch for a multi-patch effort. This patch makes nsMsgSearchTerm.cpp use the new contact service shim rather than nsIAbManager.

I'm curious if anybody has concerns about performance here, since I imagine there'll be a performance penalty due to crossing the XPConnect boundary. There's also the performance cost of converting each card to a mozIContact.

Comment 6

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

Comment on attachment 731288 [details] [diff] [review]
Patch v1

Hey Mark, does this look reasonable, and do you think you'd be the right person to review it?

Note that the next patch would attempt to have the thread pane use the new contact service to render author and recipient names.

Comment 7

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

Comment on attachment 731288 [details] [diff] [review]
Patch v1

I'm also curious what you think about this patch, jcranmer.

Comment 8

[Joshua Cranmer [:jcranmer]]

Comment on attachment 731288 [details] [diff] [review]
Patch v1

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

Also... needs more documentation comments in IDL. :-)

::: mailnews/base/search/src/nsMsgSearchTerm.cpp
@@ +999,3 @@
>  
> +  nsresult rv;
> +  nsCOMPtr<mozIContactsService> contacts = do_GetService("@mozilla.org/messenger/contacts;1", &rv);

/me is reminded of his plans to create something like Services.h for mailnews code.

@@ +1006,5 @@
> +  rv = contacts->ContactsForEmail_sync(aAddress,
> +                                       nsDependentCString(m_value.string),
> +                                       &result_count,
> +                                       &results);
> +  NS_ENSURE_SUCCESS(rv, rv);

For a minimally invasive patch, this is fine, but it's ironic to note that the API calling into our search code is fully asynchronous while the actual implementation is more or less fully synchronous.

::: mailnews/contacts/components/Makefile.in
@@ +11,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +MODULE = contacts
> +
> +EXTRA_COMPONENTS = $(wildcard $(srcdir)/*.js) \

$(wildcard) may be easier, but I think explicitly listing is better for the checkin.

::: mailnews/contacts/components/contactsService.js
@@ +33,5 @@
> +      }
> +    }
> +
> +    if (aCategoryID) {
> +      let directory = MailServices.ab.getDirectory(aCategoryID);

This isn't quite right, is it? In the card mapper, you map some of the address books to special IDs, but you don't do the inverse mapping here.

@@ +44,5 @@
> +    } else {
> +      let enumerator = MailServices.ab.directories;
> +      while(enumerator.hasMoreElements()) {
> +        let directory = enumerator.getNext();
> +        let card = directory.cardForEmailAddress(aEmailAddress);

cardForEmailAddress may throw NS_ERROR_NOT_IMPLEMENTED for a few types of address books (LDAP in particular), so you should be wrapping all calls to silently ignore at least that error.

::: mailnews/contacts/components/mozIContact.js
@@ +18,5 @@
> +}
> +
> +mozIContact.prototype = {
> +  classID: Components.ID("{fbe07bc7-094e-4d92-ac2d-230d7ec62dee}"),
> +  QueryInterface: XPCOMUtils.generateQI([Components.interfaces.mozIContact]),

(Note: this is wrong; generateQI doesn't automatically pick up mozIContactExtraProperties)

::: mailnews/contacts/modules/Makefile.in
@@ +12,5 @@
> +
> +MODULE = contacts
> +
> +libs:: $(wildcard $(srcdir)/*.jsm)
> +	$(INSTALL) $(IFLAGS1) $^ $(FINAL_TARGET)/modules/contacts

I think mozilla-central has a more general purpose INSTALL mechanism that would get us what we need here, but I don't know if it's been replicated to our build system yet. (jsmime would also love it).

::: mailnews/contacts/modules/nsIAbCardConverter.jsm
@@ +58,5 @@
> +const kPersonalAddressbookURI = "moz-abmdbdirectory://abook.mab";
> +const kCollectedAddressbookURI = "moz-abmdbdirectory://history.mab";
> +
> +
> +const nsIAbCardConverter = {

Since this is an implementation, not an interface, you should probably lose the "I".

@@ +457,5 @@
> +      }
> +      let data = NetUtil.readInputStreamToString(aInputStream,
> +                                                 aInputStream.available());
> +
> +      let blob = new Blob([data], {type: mimeType});

Is there really no File constructor for chrome-privileged code that allows us to get this from the filename? I'm uneasy about trusting this stuff with binary data...

::: mailnews/contacts/moz.build
@@ +5,5 @@
> +
> +DIRS += [
> +    'modules',
> +    'components',
> +]

Any reason why these aren't buildable in parallel?

::: mailnews/contacts/public/Makefile.in
@@ +9,5 @@
> +VPATH		= @srcdir@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +MODULE = contacts

I think the MODULE declarations are useless nowadays, especially when you have the corresponding XPIDL_MODULE.

::: mailnews/contacts/public/mozIContacts.idl
@@ +16,5 @@
> +  attribute jsval meta;
> +};
> +
> +[scriptable, uuid(2b0484fa-4157-457c-b2da-99651d6979d8)]
> +interface mozIContact : mozIContactExtraProperties {

What's the reason for splitting up mozIContact from mozIContactExtraProperties?

@@ +29,5 @@
> +  attribute AUTF8String displayName;
> +};
> +
> +[scriptable, uuid(5f733b88-6d93-4520-86e0-49115cd5a98a)]
> +interface mozIContactResult : nsISupports {

Maybe this wants to be a [function] as well? That should allow you to pass in a bare function from JS.

@@ +31,5 @@
> +
> +[scriptable, uuid(5f733b88-6d93-4520-86e0-49115cd5a98a)]
> +interface mozIContactResult : nsISupports {
> +  void handleResult(in nsresult aStatus, [optional] out unsigned long count,
> +                    [retval,array,size_is(count)] out mozIContact results);

Do you mean inparams or outparams here?

@@ +41,5 @@
> +                    [retval,array,size_is(count)] out mozIContactCategory results);
> +};
> +
> +[scriptable, uuid(7539e567-219b-4163-bf9f-fce87ef63779)]
> +interface mozIContactsService : nsISupports {

The idealist in me wants you to mark all the _sync variants as [deprecated] off the bat, which should add warnings to everyone who uses them from C++. I also subscribe to the style convention that method names should generally be verbs (i.e., "getContactsForEmail" instead of "contactsForEmail").

::: mailnews/contacts/test/unit/test_nsIAbCardConverter.js
@@ +198,5 @@
> +function blob_matches(aBlobA, aBlobB) {
> +  do_check_true(aBlobA instanceof Ci.nsIDOMBlob);
> +  do_check_true(aBlobB instanceof Ci.nsIDOMBlob);
> +  do_check_true(aBlobA instanceof Ci.nsIDOMFile);
> +  do_check_true(aBlobB instanceof Ci.nsIDOMFile);

These might die if/when Blob moves to the WebIDL bindings.

@@ +478,5 @@
> +
> +    do_check_eq(contact.fields.photo.length, 1);
> +    // We'll want to do more testing in the future, but right
> +    // now, Blob and nsIDOMFile are not readily accessible
> +    // from within XPCShell tests.

Ew... I don't like having a complex possibly-buggy photo conversion ending up being untested...

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 731288 [details] [diff] [review]
Patch v1

I don't think I've got any comments above what Joshua has already commented.

Comment 10

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

This patch has badly bitrotted. I'm going to try to get it back up to snuff.

Comment 11

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

Fuuuuuuu - the nsIDOMContactProperties.idl stuff has all gone away.

Comment 12

[Joshua Cranmer [:jcranmer]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> Fuuuuuuu - the nsIDOMContactProperties.idl stuff has all gone away.

The canonical way to refer to a WebIDL interface from XPIDL, I believe, is to label it as nsISupports. From JS, everything should just work. From C++, you have to do a manual unwrap. I'll see if I can scrounge some examples up from DXR for you.

Comment 13

[Wayne Mery (:wsmwk)]

Safe to say this is no longer needed?

Comment 14

[Magnus Melin [:mkmelin]]

Right.