Bugzilla

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? Please describe that.

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 useless comment. So, in the comment, you can replace "escaping" with a more precise reference how and what you escape, and "vcard" with the RFC and spec version that you implement.

A few unqualified and superficial peanut gallery comments:

So, this is using [ical.jsm](https://searchfox.org/comm-central/source/calendar/base/modules/Ical.jsm) from [github](https://github.com/mozilla-comm/ical.js). I was quite confused, given that there's already [ical](https://www.npmjs.com/package/ical) , so [ical.js](https://www.npmjs.com/package/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.

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 useless comment. So, in the comment, you can replace "escaping" with a more precise reference how and what you escape, and "vcard" with the RFC and spec version that you implement.

A few unqualified and superficial peanut gallery comments:

So, this is using [ical.jsm](https://searchfox.org/comm-central/source/calendar/base/modules/Ical.jsm) from [github](https://github.com/mozilla-comm/ical.js). I was quite confused, given that there's already [ical](https://www.npmjs.com/package/ical) , so [ical.js](https://www.npmjs.com/package/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.

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.

Further nit: The comment also seems to be wrong, because it says `nsIAbCard string`, but `nsIAbCard` is not a string.

A few unqualified and superficial peanut gallery comments:

So, this is using [ical.jsm](https://searchfox.org/comm-central/source/calendar/base/modules/Ical.jsm) from [github](https://github.com/mozilla-comm/ical.js). I was quite confused, given that there's already [ical](https://www.npmjs.com/package/ical) , so [ical.js](https://www.npmjs.com/package/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.

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 unqualified and superficial peanut gallery comments:

So, this is using [ical.jsm](https://searchfox.org/comm-central/source/calendar/base/modules/Ical.jsm) from [github](https://github.com/mozilla-comm/ical.js). I was quite confused, given that there's already [ical](https://www.npmjs.com/package/ical) , so [ical.js](https://www.npmjs.com/package/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.

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](https://searchfox.org/comm-central/source/calendar/base/modules/Ical.jsm) from [github](https://github.com/mozilla-comm/ical.js). I was quite confused, given that there's already [ical](https://www.npmjs.com/package/ical) , so [ical.js](https://www.npmjs.com/package/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.