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

NEW
Assigned to

Status

MailNews Core
Address Book
5 years ago
3 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Created attachment 714190 [details] [diff] [review]
WIP Patch 0.1

Checkpointing some boilerplate.
Created attachment 716332 [details] [diff] [review]
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.
Attachment #714190 - Attachment is obsolete: true
Created attachment 716334 [details] [diff] [review]
WIP patch 0.2

Whoops - included the wrong IDL version.
Attachment #716332 - Attachment is obsolete: true
Created attachment 725937 [details] [diff] [review]
WIP patch 0.3
Attachment #716334 - Attachment is obsolete: true
Created attachment 731288 [details] [diff] [review]
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.
Attachment #725937 - Attachment is obsolete: true
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.
Attachment #731288 - Flags: feedback?(mbanner)
Comment on attachment 731288 [details] [diff] [review]
Patch v1

I'm also curious what you think about this patch, jcranmer.
Attachment #731288 - Flags: feedback?(Pidgeot18)
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...
Attachment #731288 - Flags: feedback?(Pidgeot18) → feedback+
Comment on attachment 731288 [details] [diff] [review]
Patch v1

I don't think I've got any comments above what Joshua has already commented.
Attachment #731288 - Flags: feedback?(mbanner) → feedback+
This patch has badly bitrotted. I'm going to try to get it back up to snuff.
Fuuuuuuu - the nsIDOMContactProperties.idl stuff has all gone away.
(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.
You need to log in before you can comment on or make changes to this bug.