1639430 - Use ICAL.js to parse and encode vCard, remove old component

Status: RESOLVED FIXED

(MailNews Core :: Address Book, enhancement)

Description

[Geoff Lankow (:darktrojan)]

There's a lot of vCard bugs, and this is probably a duplicate of several of them, but I'm creating a new bug to be clear about exactly what I'm doing here. Once this bug is fixed I'll go over the open bugs and close as many as I can.

In this bug I'm completely replacing the C++ vCard handling with ICAL.js. We now ship this by default so there should be no issues with extensions or versions like there would've been had we done it this way in the past.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1639430-vcard-utils-1.diff

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1639430-switch-to-icaljs-vcard-1.diff

Comment 3

[Ben Bucksch (:BenB)]

Good thing to use JavaScript, for security, code sharing, etc.

You're reducing the code footprint a lot. That is great.

And at the same time, you're supporting the newer vcard format. That is fantastic.

Could you please elaborate (preferably in code/IDL comments) what you tested this with? Do we now properly import vcards exported by Android? Which versions of Android? Do all fields get imported properly? Which fields? I see you added tests or changed tests. How were the new test cases generated? I'm asking, because our old parser or generator (I don't remember the direction, sorry - I realize this is only the parser here in this patch) did not properly import/export Android vcards. It often wouldn't work at all, and also some fields were not imported or garbled. I also remember problems with the correct escaping of some fields. The field values had raw characters that one of the sides would consider invalid. Take care for newlines (e.g. in the notes field), umlauts, and special characters in the format like "=". It would be good to have test cases that expose these cases. This is where it fails in reality.

Speaking of escaping, in the IDL:

Translates a nsIAbCard string into an escaped vCard.
ACString abCardToEscapedVCard(in nsIAbCard abCard);

Could you please describe what kind of "escaping" is meant here? There are plenty of different kinds - HTML escaping, URL escaping etc. pp. Which escaping algorithm is used here? Also, what exactly is escaped? The entire file? Or just the values in the fields? Could you please expand on that in the function comment?

As a general rule for function comments, a definition must never use the word that it is defining, otherwise it's an infinite recursion :-) - or simply put not helpful. So, in the comment, you can replace "escaped" with a more precise reference how and what you escape, and "vCard" with the RFC and spec version that you implement.

Nit: The comment says nsIAbCard string, but nsIAbCard is not a string.

A few more unqualified and superficial peanut gallery comments:

So, this is using ical.jsm from github. I was quite confused, given that there's already ical , so ical.js as name is confusing. It's also a bit surprising to use ical (calendar format) to parse vcard (address book format), although of course they share a lot of the format. That suggests that ical.js might even be renamed in a way to reflect that.

It does warrant a big and prominent explanation in the implementation file why and how exactly you're using ical.js to parse vcard.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9150339 [details] [diff] [review]
1639430-vcard-utils-1.diff

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

::: mailnews/addrbook/jsaddrbook/VCardUtils.jsm
@@ +5,5 @@
> +const EXPORTED_SYMBOLS = ["VCardUtils"];
> +
> +const { ICAL } = ChromeUtils.import("resource:///modules/calendar/Ical.jsm");
> +
> +var VCardUtils = {

Please add a top level documentation like

/** 
  * Utilities for working with vCard data. 
 * @see RFC 6350.
  */

@@ +137,5 @@
> +  vPropType = "text"
> +) {
> +  return {
> +    fromAbCard(map) {
> +      if (map.has(abPropName)) {

would be nice to add jsdoc comments so perhaps in the future we can have it checked.

/** @param {Map} map - A map of address book properties to map. */

@@ +142,5 @@
> +        return [vPropName, { ...vPropParams }, vPropType, map.get(abPropName)];
> +      }
> +      return null;
> +    },
> +    toAbCard(value) {

/** @param {string} value - vCard string to map to an address book card property. */

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9150340 [details] [diff] [review]
1639430-switch-to-icaljs-vcard-1.diff

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

LGTM! So much of that old c code was barely readable. It's a pure wonder it worked to the extent it worked. Beside manual changes, looks like basically no changes at least since the dawn of hg!

::: mailnews/addrbook/jsaddrbook/VCardUtils.jsm
@@ +168,5 @@
> +
> +    let abCard = VCardUtils.vCardToAbCard(data);
> +    let escapedVCard = encodeURIComponent(data);
> +
> +    let propertiesTable = `<table id="moz-vcard-properties-table">`;

I see this id is used in the old code. I think it would be preferable to use a class still. You never know where this output might end up being used in the future.

@@ +186,5 @@
> +    return `<html>
> +      <body>
> +        <table class="moz-vcard-table">
> +          <tr>
> +            <td valign="top"><a class="moz-vcard-badge" href="addbook:add?action=add?vcard=${escapedVCard}"></a></td>

Existing code, but multiple question-marks in the url - ouch!

::: mailnews/addrbook/public/nsIMsgVCardService.idl
@@ +15,2 @@
>     *
> +   * @param  escapedVCardStr  The string containing the vCard.

nit: odd spaces here. same below.
 
And I suppose it's the vCard *data*

Comment 6

[Geoff Lankow (:darktrojan)]

(In reply to Ben Bucksch (:BenB) from comment #3)
A major caveat: This is a step towards the goal of adding CardDAV support in 78, not the goal itself. The older code is largely unusable for this purpose. We want to get CardDAV in front of a test audience ASAP so I'm doing only what we need right now. Even with these changes there will still be issues to deal with we're probably in a better place but certainly not in a worse place.

Could you please elaborate (preferably in code/IDL comments) what you tested this with? Do we now properly import vcards exported by Android? Which versions of Android? Do all fields get imported properly? Which fields? I see you added tests or changed tests. How were the new test cases generated? I'm asking, because our old parser or generator (I don't remember the direction, sorry - I realize this is only the parser here in this patch) did not properly import/export Android vcards. It often wouldn't work at all, and also some fields were not imported or garbled. I also remember problems with the correct escaping of some fields. The field values had raw characters that one of the sides would consider invalid. Take care for newlines (e.g. in the notes field), umlauts, and special characters in the format like "=". It would be good to have test cases that expose these cases. This is where it fails in reality.

I'd love to have more test cases but that is time-consuming and I don't have a lot of time right now. I have in fact been looking for sample files I can import into our tree to test with but they are hard to find. Interoperability with Android still needs work (for new reasons) and I fully intend to do that.

We're only importing fields we currently use, though adding more when we have some way of interacting with them should be pretty easy. With CardDAV we'll be keeping the original vCard alongside the imported data, and using that for updates so that no data is lost even if it's not explicitly stored in our database.

Could you please describe what kind of "escaping" is meant here? There are plenty of different kinds - HTML escaping, URL escaping etc. pp. Which escaping algorithm is used here? Also, what exactly is escaped? The entire file? Or just the values in the fields? Could you please expand on that in the function comment?

It's URL-escaping of the whole file. That's how it was done before, presumably for the questionable ways we deal with vCards in a message.

So, this is using ical.jsm from github. I was quite confused, given that there's already ical , so ical.js as name is confusing. It's also a bit surprising to use ical (calendar format) to parse vcard (address book format), although of course they share a lot of the format. That suggests that ical.js might even be renamed in a way to reflect that.

Don't look at me, I didn't name it. :-)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/450c5f1fed90
Create utility functions to use ICAL.js for parsing and encoding vCard. r=mkmelin
https://hg.mozilla.org/comm-central/rev/299e88e21913
Use ICAL.js to parse and encode vCard; remove old vCard components. r=mkmelin DONTBUILD

Comment 8

[Ben Bucksch (:BenB)]

a step towards the goal

Understood.

we'll be keeping the original vCard alongside the imported data, and using that for updates so that no data is lost even if it's not explicitly stored in our database.

Good. No data loss in round trips is crucially important. I've been victim of that too many times, with various implementations.

It's URL-escaping of the whole file

a) Thanks. That statement makes it much clearer. Could you please state exactly that in the function comment?
b) I don't see the point of this function doing that. Could the caller do that? How about a function ACString abCardToVCard(in nsIAbCard abCard); that returns the unescaped vCard? And for short-term API compat, just abCardToEscapedVCard(abCard) => urlEncode(abCardToVCard(abCard));? That seems like a saner API to me.

Comment 9

[Ben Bucksch (:BenB)]

Hey geoff,

I just ran across https://sabre.io/dav/building-a-carddav-client/ , which is highly educating.

Particularly, it says:

Different escaping mechanisms of values, which depends on the name of the value.
Parameters, with different escaping mechanisms and a new (rfc6868) standard escaping mechanism that noone supports yet.

So, this is even way more complex than I thought. This gives additional weight to comment 8.

Generally, I think you will find that doc very enlightening.
There are lots of pitfalls, and it's good to avoid the gotchas before they destroy user data.

I've very much looking forward to CardDAV support. In fact, it's the number one missing feature of TB for me. So, thanks for working on that!

Comment 10

[Дилян Палаузов]

You might want to test, if folding lines with UTF-16 high and low surrogate, does work with ical.js.

This is addressed at https://github.com/mozilla-comm/ical.js/pull/439 in more details.