Last Comment Bug 79709 - import vcards
: import vcards
Status: RESOLVED FIXED
ucosp
:
Product: MailNews Core
Classification: Components
Component: Address Book (show other bugs)
: Trunk
: All All
: -- normal with 74 votes (vote)
: Thunderbird 3.3a1
Assigned To: Evan Stratford
:
:
Mentors:
: 163108 176803 240332 271634 354207 630596 (view as bug list)
Depends on: 602188 717736
Blocks: 600798 603265
  Show dependency treegraph
 
Reported: 2001-05-09 08:00 PDT by Ken Guest
Modified: 2012-08-20 08:44 PDT (History)
43 users (show)
standard8: in‑testsuite+
ludovic: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Adds vCard import to Address Book importer. (57.52 KB, patch)
2010-02-24 20:13 PST, Evan Stratford
no flags Details | Diff | Splinter Review
Modified patch, taking into account the comments from Standard8. (49.22 KB, patch)
2010-02-25 14:20 PST, Evan Stratford
no flags Details | Diff | Splinter Review
switching to string-based indices for message bundle (51.54 KB, patch)
2010-03-06 13:05 PST, Evan Stratford
standard8: review-
Details | Diff | Splinter Review
Addressing comments from standard8 (48.39 KB, patch)
2010-04-01 16:38 PDT, Evan Stratford
no flags Details | Diff | Splinter Review
Fixed up (42.67 KB, patch)
2010-09-15 08:34 PDT, Mark Banner (:standard8, limited time in Dec)
standard8: review+
mozilla: superreview+
Details | Diff | Splinter Review

Description Ken Guest 2001-05-09 08:00:41 PDT
it would be very useful to be able to import (File| Import | Addressbook) vcard
entries.
Comment 1 Asa Dotzler [:asa] 2001-05-09 18:51:04 PDT
updating product and component
Comment 2 Ken Guest 2001-05-09 22:31:58 PDT
I don't know if this should be split up into a seperate bug report, it is also 
to do with importing vcard information, but triggered by downloading a file 
of text/x-vCard mime type:

on a similar note, it would be most useful if vcards could also be
imported/added to the address book when the user downloads/clicks on a file of
text/x-vCard mime type. If the user wants that file saved to the harddrive (i.e.
not import it) he can always do a right-click and choose "save file as" from the
popup menu.
Comment 3 J Luh 2001-06-20 20:45:24 PDT
This is pretty similar to bug 38970 "Ability to Add vcard from browser"
Comment 4 Karl Ove Hufthammer 2002-01-13 07:24:46 PST
Note that the correct MIME type is 'text/directory', not 'text/x-vCard'. See
RFC 2425.
Comment 5 Jo Hermans 2002-08-16 12:58:55 PDT
*** Bug 163108 has been marked as a duplicate of this bug. ***
Comment 6 (not reading, please use seth@sspitzer.org instead) 2002-09-07 17:16:06 PDT
taking all of chuang's bugs.  she doesn't work on mozilla anymore.
Comment 7 (not reading, please use seth@sspitzer.org instead) 2003-10-06 08:35:57 PDT
scott, does BMS require this?
Comment 8 John Mora 2003-10-24 19:05:56 PDT
I'm considering switching to Thunderbird and have a large personal addressbook
in Palm Desktop that can be exported as one large, multi-entry vcard.

By this, I mean a single vcard file with several people listed in it. It can be
imported into Evolution's addressbook without a problem. (Evolution recognizes
it as being in GnomeCard/vCard/VCF format) and the field names and data
associated with them are properly handled.

I would like to do the same with Thunderbird - that is, import this single
multi-entry vcard into my Thunderbird addressbook. I currently have no way to
migrate my addressbook into TB.

Ken Guest summarized the issue best in Comment #2.
Comment 9 Troy Miller 2004-03-29 14:53:33 PST
(In reply to comment #8)
> I would like to do the same with Thunderbird - that is, import this single
> multi-entry vcard into my Thunderbird addressbook. I currently have no way to
> migrate my addressbook into TB.

I would like this too. However, since we don't have it yet, I'll point out that
there *is* a workaround, albeit a ReallyAnnoying(tm) one. Plus, you need Windows.

Basically, you just need to import your address book into Outlook Express (I
assume Outlook or Eudora would work as well), then import from *that* into
Thunderbird. In my case, I didn't even have to start OE - I just clicked on the
vcard file in Windows which brought up the default viewer, and I went through
and clicked "Add to Address Book" on each of them, then went to Thunderbird and
imported from Outlook Express.

Of course, this theory should work if you can get your vCard format into
anything else that TB recognizes.

HTH.
Comment 10 Matthias Versen [:Matti] 2004-04-13 08:04:07 PDT
*** Bug 240332 has been marked as a duplicate of this bug. ***
Comment 11 Ian McCall 2004-07-25 15:52:31 PDT
Hi.

No promises, but I'm considering having a crack at this one as my first piece of Thunderbird coding and 
would appreciate some pointers.

I've downloaded the source to Thunderbird 0.7.1 and have looked at mozilla/mailnews/addrbook and 
mozilla/mailnews/import directories. Seems to me that if I add my handler inside the import, then 
create something that loops through a file and creates vCard objects as specified in the addrbook 
section then I should be able to complete this.

As far as I can tell, I should be opening up a stream, and looking at nsMsgVCardService to process that 
stream. Questions...


1. Is the above right?
2. Do I need to create the stream, or will some existing dialog pass it to me? What's the entry point I 
must define if the stream gets passed to me?
3. How do I then go about commiting the found objects to the address book?
4. Are there certain required fields that I absolutely -must- have filled in before the address book will 
allow commits?


Should mention my reason for doing this: I'm a Mail.app/Address Book user on OS X, and a 
Thunderbird user on Windows. Would like to sync my address books between the two machines.


Cheers,
Ian
Comment 12 Marian 2004-12-12 06:50:19 PST
I was looking for a way to import/export addresses between Thunderbird (1.0) and
my Sony Ericsson T630 mobile phone. The phone is capable of sending the whole
address book as a single vCard file (via bluetooth or infrared), which I thought
would help me a lot.

However, now it depends on Thunderbird beeing able to import this. I actually
considered this a must-have for Thunderbird.

Developers, note: Import options are vital in order to make an application more
popular. :) Just a user's $ 0.02.
Comment 13 Jörg Thönnes 2005-02-17 04:45:06 PST
Still missing the ability to import vCards.

This is a *MUST* if users should switch from other browsers/mail clients to use
Mozilla/TB

Cheers, Jörg
Comment 14 pioterus 2005-02-17 09:52:39 PST
Exactly. All, not only claiming to be modern, mail clients have better 
handling of vCards than Tb. Also it is often the only format for exchanging 
data with many mobiles, e.g. my own Siemens and is accepted by all modern 
ones. I wonder why after more than three years after this bug had been filed, 
this really basic and uncomplicated feature hasn't been yet implemented. 
Microsoft, which by Mozilla Found. is said not to comply with open standards, 
has included this factually for years industial standard in its products. Is 
anything at all going to be done about it?
Comment 15 David :Bienvenu 2005-02-17 11:23:49 PST
Robert, you must have written code that basically does this, right?
Comment 16 Robert Accettura [:raccettura] 2005-02-17 12:36:09 PST
I've done export, as it applies to the iPod (iPod doesn't support multiple
vcards in 1 file, as it's not 'to spec' according to Apple's Engineers), and bug
221991, among a few little things.

Import should already exist.... just no UI for it.

when I receive an email with a vcard attached, I can import it by clicking on it.

IMHO that's the right place to work from.  Check with mscott, as I recall he
implemented that during Tbird, somewhat early as I recall.
Comment 17 Jörg Thönnes 2005-02-18 02:48:27 PST
IMHO, just adding vCards from received e-mails is not sufficient.
It should be also possible to import vCard directly from a file into the
addressbook, just in the same way as this can be done with LDIF or TXT files.

Cheers, Jörg
Comment 18 S Bufton 2005-02-19 03:37:08 PST
My wife just switched *from* Mozilla *to* Outlook because this feature is
missing in Mozilla/Thunderbird.
Comment 19 Robert Accettura [:raccettura] 2005-02-19 20:41:06 PST
To summarize a quick conversation with bienvenu:

1.  We don't support vcard 3.0... we should.
2.  We have a addressboook: method to import a card.  We don't have a UI, or a
way to do them in bulk.

As part of 1, we need to decide if we will support multiple vcards in 1 file
(inline with 2 returns separating them).  Apple told me they don't support it
because they don't believe it's part of the Spec.  I've confirmed that PalmOne
(formerly Palm Inc.) does support this in Palm Desktop.

That of course is another bug, and regards both import and export.

vCard is slightly a mess... but the groundwork is there.  It's a matter of
catching up and finishing what was started.

Creating import could be done using addbook:add?action=add?vcard=<<vcard
urlencoded()>>

So it's a matter of looping through and doing that.  It should be feasable with
just some JS and some XUL changes.

Not sure if I'll have free time to do this in the near future.  David and Scott
would be much more likely.  Though I'll keep it in mind if I get free cycles.


Please refrain from unnecessary comments.  We know this is a desired feature. 
Add your vote if you wish, but don't keep the "me too" comments.  It doesn't
help anyone.
Comment 20 Jörg Thönnes 2005-02-21 00:51:26 PST
Thanks, Robert.

But most of the people (as do I) had the feeling that they need some feedback
whether you are still working on this.

Actually some tags as Target Milestone etc. would indicate this easily in the
bugtracking system and in this way stopping to people to ask when there haven't
been any comments for a while.

Cheers, Jörg
Comment 21 Robert Accettura [:raccettura] 2005-02-21 09:42:22 PST
There is no target set... since nobody is working on this.
Comment 22 Ian McCall 2005-02-21 12:50:01 PST
I offered to work on it a few comments up - would be my first bit of Thunderbird coding though. 
Anyone care to see if I was on the right track and perhaps give a few pointers?


Ian
Comment 23 Mark Banner (:standard8, limited time in Dec) 2005-04-16 08:47:39 PDT
I had a word with Seth and he isn't working on this at the moment. Therefore
returning status to new.
Comment 24 db 2005-07-19 08:00:09 PDT
Another workaround is to attach a vCard you want to import to a new (outgoing)
message and then click it and it will be added to your address book.
Comment 25 Antonio M. D'souza 2005-07-20 03:51:03 PDT
But will that work for a vCard with multiple entries?
Comment 26 Mark Banner (:standard8, limited time in Dec) 2005-08-18 02:35:14 PDT
*** Bug 271634 has been marked as a duplicate of this bug. ***
Comment 27 Bill McGonigle (not currently reading bugmail; please contact directly) 2006-08-08 21:23:05 PDT
(In reply to comment #19)
> As part of 1, we need to decide if we will support multiple vcards in 1 file
> (inline with 2 returns separating them).  Apple told me they don't support it
> because they don't believe it's part of the Spec.

Just in case this isn't already known, Apple Address Book will export multiple VCards into a single file if you select multiple records and Export.  I don't see two returns between records, just:

END:VCARD
BEGIN:VCARD
Comment 28 Adam Guthrie 2006-09-25 18:07:31 PDT
*** Bug 354207 has been marked as a duplicate of this bug. ***
Comment 29 Sven Strickroth 2007-03-27 03:33:05 PDT
Isn't this the same as bug 176803?
Comment 30 Magnus Melin 2007-03-27 10:56:00 PDT
*** Bug 176803 has been marked as a duplicate of this bug. ***
Comment 31 Felix Möller 2007-10-14 05:48:22 PDT
This would be an important feature to get synching working with Thunderbird. As OpenSync gets better more and more Linux-Users will want to import their VCards.
Comment 32 Niko 2008-02-20 05:55:13 PST
isnt this blocked by bug 266891 ? Right now you just cant import/export vcards correctly, because of the inflexibility of the current adress book (ie. you cant save the vcard uid right now, which is a requirement, otherwise you will have tons of duplicates)
Comment 33 Mark Banner (:standard8, limited time in Dec) 2008-02-20 14:25:32 PST
(In reply to comment #32)
> isnt this blocked by bug 266891 ? Right now you just cant import/export vcards
> correctly, because of the inflexibility of the current adress book (ie. you
> cant save the vcard uid right now, which is a requirement, otherwise you will
> have tons of duplicates)
> 
No. A vcard import could be done with the current address book structure. It would be possible to save the uid if done right as well (though only for local address books).
Comment 34 Reed Hedges 2008-09-30 13:56:46 PDT
According to the comments on this bug, Thunderbird has had some kind of vCard import ability for years, but just doesn't have it in the import UI?  Can someone look at adding this? Or find out what the difficulties might be in the current codebase? Or give pointers on how to apporoach it for someone completely new to working on Thunderbird?  Thanks
Comment 35 Reed Hedges 2008-09-30 14:18:12 PDT
I found a perl script that converts a vcard file (with multiple vcards possible) to LDIF: 

http://stud4.tuwien.ac.at/~e0325716/vcard2ldif.html

However at least with my vcard file it resulted in blank "display" names, until I edited the perl script and added back in a commented out section labelled "# CREATE A DISPLAY NAME # BY KLOKIE" and changed the definition of $change_names to assign it 1 instead of 0.
Comment 36 Niko 2008-09-30 14:31:10 PDT
(In reply to comment #33)
> (In reply to comment #32)
> > isnt this blocked by bug 266891 ? Right now you just cant import/export vcards
> > correctly, because of the inflexibility of the current adress book (ie. you
> > cant save the vcard uid right now, which is a requirement, otherwise you will
> > have tons of duplicates)
> > 
> No. A vcard import could be done with the current address book structure. It
> would be possible to save the uid if done right as well (though only for local
> address books).

I wrote an extension (synckolab) that imports vcards (and kolab xml) using imap - and I tried out quite a few things - but storing an uid was impossible for me - thats the reason why I had to use the 3rd custom field for the uid.

if anyone knows how I could write in a (hidden) uid field I could change this behavior
Comment 37 Scott 2009-01-09 00:20:44 PST
Importing and exporting a vCard would be useful.  This way you can, import one and assign it to a present account, or export one for backup purposes.
Comment 38 Matěj Cepl 2009-08-31 04:16:33 PDT
Isn't this duplicate/blocking of bug 29106?
Comment 39 Evan Stratford 2010-01-15 12:49:00 PST
Looks like it's been a while since last post here :)

As noted in https://bugzilla.mozilla.org/show_bug.cgi?id=29106, I'm taking a look at adding support for vCard import/export in Address Book.

@Matej: As I understand it, this would logically be part of that bug.
Comment 40 Evan Stratford 2010-01-27 11:32:33 PST
I've essentially got something working here - just have to clean it up and add tests, then I'll post a patch for review :)
Comment 41 Aleksander Adamowski 2010-01-27 14:16:46 PST
Yaaay!!! :)
Comment 42 Evan Stratford 2010-02-24 20:13:32 PST
Created attachment 428865 [details] [diff] [review]
Adds vCard import to Address Book importer.

First patch submit; if there's a problem, please let me know. To test, make xpcshell-tests in mailnews/import/test.
Comment 43 Mark Banner (:standard8, limited time in Dec) 2010-02-25 00:59:28 PST
Comment on attachment 428865 [details] [diff] [review]
Adds vCard import to Address Book importer.

Thanks for the patch, it looks like a very good start. I may not be able to get to review it until sometime late next week, but here's some comments I noticed from a quick look.


>+# Short name of import module
>+## @name VCARDIMPORT_NAME
>+## @loc None
>+2000=vCard file (.vcf)

I know this is what the existing code does, however we are really trying to avoid using numeric ids for strings, as it makes it harder for localisers to do their job. Can you change these into names? e.g. vcardImportName

>+    dump("\n\n\n\n*******\nsupportsMultiple: " +
>+        addInterface.GetStatus( "supportsMultiple"));

This looks like extra debug code that we don't need.
>+
>+#ifdef NS_DEBUG
>+#define IMPORT_DEBUG  1
>+#endif
>+
>+// Use PR_LOG for logging.
>+#include "prlog.h"
>+extern PRLogModuleInfo *VCARDLOGMODULE;  // Logging module
>+
>+#define IMPORT_LOG0(x)          PR_LOG(VCARDLOGMODULE, PR_LOG_DEBUG, (x))
>+#define IMPORT_LOG1(x, y)       PR_LOG(VCARDLOGMODULE, PR_LOG_DEBUG, (x, y))
>+#define IMPORT_LOG2(x, y, z)    PR_LOG(VCARDLOGMODULE, PR_LOG_DEBUG, (x, y, z))
>+#define IMPORT_LOG3(a, b, c, d) PR_LOG(VCARDLOGMODULE, PR_LOG_DEBUG, (a, b, c, d))

Can these just be defined in nsVCardAddress.h?

>+  /* PRBool GetAutoFind (out wstring description); */
>+  NS_IMETHOD GetAutoFind(PRUnichar **description, PRBool *_retval);


I don't think we need to have the comments about what's in the idl. Most devs can look it up or work it out.

>diff -r cbdc1439e07d -r 29bc2cdd8b27 mailnews/import/vcard/src/nsVCardStringBundle.cpp
>--- /dev/null
>+++ b/mailnews/import/vcard/src/nsVCardStringBundle.cpp
>diff -r cbdc1439e07d -r 29bc2cdd8b27 mailnews/import/vcard/src/nsVCardStringBundle.h
>--- /dev/null
>+++ b/mailnews/import/vcard/src/nsVCardStringBundle.h

In bug 131040 we've been trying to remove the duplicate string bundles, and re-use nsImportStringBundle. Can you do that here as well please.

Attachment 260454 [details] [diff] shows an example where I did this for the text import.

Note you may need to add a couple of GetStringByName/GetStringFromName functions, to support the switch from numeric ids to names that I mentioned above, but I'd much rather that was done in the existing class, rather than duplicating the functions again.


As I've said at the start this has been a quick look set of comments, and hopefully I'll get back to it somewhere around the middle of next week, but if you have time to update the patch in-between, that would be great.
Comment 44 Evan Stratford 2010-02-25 14:20:30 PST
Created attachment 428990 [details] [diff] [review]
Modified patch, taking into account the comments from Standard8.

Removed nsVCardStringBundle and moved nsVCardDebugLog macros into nsVCardAddress.h, as per comments from Standard8.
Comment 45 Evan Stratford 2010-03-06 13:05:13 PST
Created attachment 430871 [details] [diff] [review]
switching to string-based indices for message bundle

Switching to string-based indices for vCard message properties; adding nsImportStringBundle::GetStringByName() to support this.
Comment 46 Nikolay Shopik 2010-03-10 12:55:19 PST
Don't forget mark it as patch and put review? for Mark (bugzilla@standard8.plus.com)
Comment 47 Mark Banner (:standard8, limited time in Dec) 2010-04-01 09:02:58 PDT
Comment on attachment 430871 [details] [diff] [review]
switching to string-based indices for message bundle

Sorry for the big delay in reviewing this.

Unfortunately, the import code on which you based your patch isn't the best style, and for new code we prefer to use the more consistent newer style.

Therefore I've broken up the comments into two parts. The first are significant comments that affect functionality, performance etc, the second are comments about the style.

If you can at least fix the first set of comments that would be good. If you've got time to fix some of all of the second set, that would be very useful.

Here are the comments about functionality/performance/correctness etc:

>+++ b/mailnews/import/test/unit/test_csv_import.js
>+++ b/mailnews/import/test/unit/test_vcard_import.js

In debug mode, these tests are failing with:

###!!! ASSERTION: nsVariant not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/moztest/comm/main/src/mozilla/xpcom/ds/nsVariant.cpp, line 1694

I'm pretty sure this is due to the import running on one thread, but something in the address book saving not being proxied to the main thread - the mork database code has to run on the main thread. I haven't had chance to look at what it is that is causing this yet, but getting that in a debugger should help. I'll try and do that later if I get time.

>diff -r cbdc1439e07d -r bde920ee53b7 mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties
>diff -r cbdc1439e07d -r bde920ee53b7 mail/locales/en-US/chrome/messenger/importMsgs.properties

You're missing the suite/ versions of changes to these files.

>+++ b/mail/locales/en-US/chrome/messenger/vCardImportMsgs.properties

Based on conversations I've had, drop the license header in this file (and the suite/ equivalent) for now.

>+# Short name of import module
>+## @name VCARDIMPORT_NAME
>+## @loc None
>+vCardImportName=vCard file (.vcf)

I think for all of these we can drop the old-style comments. The string names give a good idea to l10n as to what they represent.

>+vCardImportAddressSuccess=Imported address book %S
>+vCardImportAddressBadSourceFile=Error accessing file for address book %S.
>+vCardImportAddressConvertError=Error importing address book %S, all addresses may not have been imported.

These strings should all have a localisation note similar to:

# LOCALIZATION NOTE (vCardImportAddressSuccess): %S is replaced by the name of the address book being imported.

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/build/Makefile.in
>+		../vcard/src/$(LIB_PREFIX)impVCard_s.$(LIB_SUFFIX) \

The general standard is that we have all our lib names as lower case, hence...

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/vcard/src/Makefile.in
>+MODULE		= impVcard
>+LIBRARY_NAME   = impVCard_s

...please make the impVCard lower case.

>+# The Initial Developer of the Original Code is
>+# Netscape Communications Corporation.
>+# Portions created by the Initial Developer are Copyright (C) 2001
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):

This and the other license headers need updating, I suspect you're not working for Netscape and the date needs updating.

>+nsresult nsVCardAddress::ReadRecord(
>+    nsILineInputStream *aLineStream, nsCString &aRecord, PRBool *aMore)
>+{

>+  // read BEGIN:VCARD
>+  rv = aLineStream->ReadLine(line, &more);
>+  if (!(more && line.Equals("BEGIN:VCARD"))) {
>+    rv = NS_ERROR_FAILURE;
>+    *aMore = more;
>+    return rv;
>+  }

If I'm reading this right, then it won't allow there to be one or more blank lines between END:VCARD and BEGIN:VCARD. Is that necessary? I'm just thinking about people copy and pasting cards into a file and not cleaning up.

It would also be good to add a log output here, so we know why this is failing.

>+  // read until END:VCARD
>+  while (true) {
>+    if (!more) {
>+      rv = NS_ERROR_FAILURE;
>+      break;
>+    }
>+    rv = aLineStream->ReadLine(line, &more);
>+    if (!aRecord.IsEmpty())
>+      aRecord.AppendLiteral(MSG_LINEBREAK);
>+    aRecord.Append(line);
>+    if (line.Equals("END:VCARD"))
>+      break;
>+  }

I'd suggest making this a do...while loop with the exit condition being the (line.Equals("END:VCARD")).

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/vcard/src/nsVCardAddress.h

>+  PRInt32 m_LFCount;
>+  PRInt32 m_CRCount;

m_LFCount and m_CRCount are initialised only. So these can be dropped.

>+static NS_DEFINE_IID(kISupportsIID, NS_ISUPPORTS_IID);

Drop this, I'll explain why below.

>+
>+  *name = nsImportStringBundle::GetStringByName(
>+      VCARDIMPORT_NAME, m_stringBundle);

Let's skip the #define for VCARDIMPORT_NAME and just put the string in here. Having the define doesn't actually gain us anything apart from a bit more complexity.

(ditto in the GetDescription function).

>+NS_IMETHODIMP nsVCardImport::GetImportInterface(
>+    const char *pImportType, nsISupports **ppInterface)

>+  nsresult rv;
>+  *ppInterface = nsnull;
>+  if (!strcmp( pImportType, "addressbook")) {

rv can be declared after within the if statement as its only used in that block.

>+          rv = pGeneric->QueryInterface(kISupportsIID, (void **)ppInterface);

This would be much better as:

ppInterface = do_QueryInterface(pGeneric, &rv);

(it also enforces type safety).

>+NS_IMETHODIMP ImportVCardAddressImpl::FindAddressBooks(
>+    nsIFile *pLoc, nsISupportsArray **ppArray)

>+    rv = desc->QueryInterface( kISupportsIID, (void **) &pInterface);

This should use the do_QueryInterface form.

>+    rv = array->QueryInterface(
>+        NS_GET_IID(nsISupportsArray), (void **) ppArray);

As 'array' is already an nsISupportsArray, we can very simply make this:

array.swap(*ppArray);

Which also avoids an additional reference count.
Comment 48 Mark Banner (:standard8, limited time in Dec) 2010-04-01 09:04:22 PDT
Comment on attachment 430871 [details] [diff] [review]
switching to string-based indices for message bundle

Here are the second set of comments about the style nits/issues.

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/build/nsImportModule.cpp

>+  if (NS_SUCCEEDED( rv)) {

nit: no space before rv (please apply generally in other places as well).

>+    NS_Free( theCID);

nit: no space before "theCID".

>+  return( rv);

nit: just drop the brackets here.

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/vcard/src/Makefile.in

>+CPPSRCS		= \
>+    nsVCardAddress.cpp \
>+    nsVCardImport.cpp \
>+		$(NULL)

I think we should format this like:

CPPSRCS = \
  nsVCardAddress.cpp \
  nsVCardImport.cpp \
  $(NULL)

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/vcard/src/nsVCardAddress.cpp

>+nsresult nsVCardAddress::ImportAddresses(

>+nsresult nsVCardAddress::ProcessRecord(nsCString& aRecord, nsString& errors)
...
>+  return( rv);

nit: no brackets here.

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/vcard/src/nsVCardAddress.h

>+/////////////////////////////////////////////////////////////////////////////////////
>+/////////////////////////////////////////////////////////////////////////////////////
>+/////////////////////////////////////////////////////////////////////////////////////

Drop these comments, they aren't necessary.

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/vcard/src/nsVCardImport.cpp

>+  NS_IMETHOD GetSupportsMultiple(PRBool *_retval)
>+  { *_retval = PR_FALSE; return( NS_OK);}

>+  NS_IMETHOD GetNeedsFieldMap(nsIFile *location, PRBool *_retval)
>+  { *_retval = PR_FALSE; return( NS_OK);}

>+  NS_IMETHOD InitFieldMap(nsIImportFieldMap *fieldMap)
>+  { return( NS_ERROR_FAILURE);}

>+  NS_IMETHOD GetSampleData( PRInt32 index, PRBool *pFound, PRUnichar **pStr)
>+  { return( NS_ERROR_FAILURE);}
>+
>+  NS_IMETHOD SetSampleLocation( nsIFile *)
>+  { return( NS_ERROR_FAILURE); } 

nit: no brackets around the return values please. Please apply this to all other places than just the ones I've picked out here.

>+  IMPORT_LOG0( "nsVCardImport Module Created\n");

nit: no space after (. Please fix this in multiple places.

>+NS_IMETHODIMP nsVCardImport::GetName( PRUnichar **name)

Ditto here, but for function calls.

>+{
>+  NS_PRECONDITION(name != nsnull, "null ptr");
>+  if (! name)
>+    return NS_ERROR_NULL_POINTER;

You can replace this by:

NS_ENSURE_ARG_POINTER(name);

Please do this with similar patters in the rest of the patch.

>+NS_IMETHODIMP nsVCardImport::GetDescription( PRUnichar **name)
>+NS_IMETHODIMP nsVCardImport::GetSupports( char **supports)
>+NS_IMETHODIMP nsVCardImport::GetSupportsUpgrade( PRBool *pUpgrade)

Please apply similar comments to these functions as per the ones I've given on the GetName function.

>+NS_IMETHODIMP nsVCardImport::GetImportInterface(
>+    const char *pImportType, nsISupports **ppInterface)

I should have mentioned this earlier, but can you make all of these function definitions formatted like this:

NS_IMETHODIMP
nsVCardImport::GetImportInterface(const char *pImportType,
                                  nsISupports **ppInterface)

>+  NS_PRECONDITION(pImportType != nsnull, "null ptr");
>+  if (! pImportType)
>+    return NS_ERROR_NULL_POINTER;
>+  NS_PRECONDITION(ppInterface != nsnull, "null ptr");
>+  if (! ppInterface)
>+    return NS_ERROR_NULL_POINTER;

Again, these need replacing.

>+    NS_IF_RELEASE( pAddress);
>+    NS_IF_RELEASE( pGeneric);
>+    return( rv);

no space after ( and no brackets around the return value.

>+////////////////////////////////////////////////////////////////////////

We can drop this comment.

>+  *aImport = new ImportVCardAddressImpl(aStringBundle);
>+  if (! *aImport)
>+    return NS_ERROR_OUT_OF_MEMORY;

nit: no space after !

>+NS_IMETHODIMP ImportVCardAddressImpl::FindAddressBooks(
>+    nsIFile *pLoc, nsISupportsArray **ppArray)

>+  nsCOMPtr<nsIImportABDescriptor>  desc;
>+  nsISupports * pInterface;

nit: no space after *.

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/vcard/src/nsVCardImport.h

>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

This should be tab-width: 2. Please make sure its the same in other places.
Comment 49 Evan Stratford 2010-04-01 16:38:33 PDT
Created attachment 436596 [details] [diff] [review]
Addressing comments from standard8

Addressing most of the comments above, along with a bit of general cleanup. After some discussion on IRC, it seems these thread-safety issues are pretty well entrenched in the importing framework...
Comment 50 Mark Banner (:standard8, limited time in Dec) 2010-04-09 06:31:14 PDT
Comment on attachment 436596 [details] [diff] [review]
Addressing comments from standard8

>+ *
>+ * The Original Code is mozilla.org Code.
>+ *
>+ * Contributor(s):
>+ *   Evan Stratford <evan.stratford@gmail.com>
>

You're missing this section (which is required):

 * The Initial Developer of the Original Code is
 * ____________________________________________.
 * Portions created by the Initial Developer are Copyright (C) 2___
 * the Initial Developer. All Rights Reserved.
 *
 * Contributor(s):

You can put the initial developer as yourself (and fill in the year)

Also the vcard test is failing with:

TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | TypeError: importService.GetModule(aModuleName, i).GetImportInterface(aModuleName) is null - See following stack:

This patch is basically r+, but I'm still thinking about the threadsafe issues and the tests. I suggest you update it for the above fixes and request sr from :bienvenu. In the meantime, I'll try out something for fixing the threadsafe issues.
Comment 51 Mark Banner (:standard8, limited time in Dec) 2010-09-15 08:34:27 PDT
Created attachment 475533 [details] [diff] [review]
Fixed up

Sorry Evan, I really should have got to this ages ago. What I've just realised is that we said if unit tests were too hard/complex we could push them out to other things and still land the patch.

So I'm proposing that we land this with some litmus testing, and push out the issues with the threadsafey in the tests to another bug.

Therefore I've updated this to resolve the bitrot and the couple of remaining issues and asking David to take a quick look over.
Comment 52 Mark Banner (:standard8, limited time in Dec) 2010-09-30 01:41:08 PDT
I've moved the test cases out to bug 600798. Ludovic: we should get something in litmus in the meantime.

This bug is now landed: http://hg.mozilla.org/comm-central/rev/9021466c11d8
Comment 53 Ludovic Hirlimann [:Usul] 2010-09-30 01:56:43 PDT
Evan what version of vcard are we supporting with this patch ?
Comment 54 Jens Hatlak (:InvisibleSmiley) 2010-09-30 04:59:23 PDT
(In reply to comment #52)
> I've moved the test cases out to bug 600798. Ludovic: we should get something
> in litmus in the meantime.
> 
> This bug is now landed: http://hg.mozilla.org/comm-central/rev/9021466c11d8

Looks like a change to suite/locales/jar.mn is missing. Otherwise thanks for taking care of SeaMonkey! :-)
Comment 55 Mark Banner (:standard8, limited time in Dec) 2010-09-30 06:37:45 PDT
(In reply to comment #54)
> (In reply to comment #52)
> > I've moved the test cases out to bug 600798. Ludovic: we should get something
> > in litmus in the meantime.
> > 
> > This bug is now landed: http://hg.mozilla.org/comm-central/rev/9021466c11d8
> 
> Looks like a change to suite/locales/jar.mn is missing. Otherwise thanks for
> taking care of SeaMonkey! :-)

Fixed: http://hg.mozilla.org/comm-central/rev/24dafd19672c
Comment 56 Jens Hatlak (:InvisibleSmiley) 2010-09-30 06:42:38 PDT
(In reply to comment #55)
> (In reply to comment #54)
> > (In reply to comment #52)
> > > I've moved the test cases out to bug 600798. Ludovic: we should get something
> > > in litmus in the meantime.
> > > 
> > > This bug is now landed: http://hg.mozilla.org/comm-central/rev/9021466c11d8
> > 
> > Looks like a change to suite/locales/jar.mn is missing. Otherwise thanks for
> > taking care of SeaMonkey! :-)
> 
> Fixed: http://hg.mozilla.org/comm-central/rev/24dafd19672c

Nit: Missing ) ;-)
Comment 57 Rimas Kudelis 2010-10-01 13:56:30 PDT
I can only see vCardImportAddressBadParam in the language files in the patch. Is that string unused?
Comment 58 neil@parkwaycc.co.uk 2010-10-03 14:46:36 PDT
Comment on attachment 475533 [details] [diff] [review]
Fixed up

[The commit date for this patch was 1 Apr, is this some kind of joke?]

>+  if (!line.EqualsIgnoreCase("BEGIN:VCARD")) {
[EqualsIgnoreCase doesn't exist in the external API; I can't remember but maybe
 we could use Equals("BEGIN:VCARD", CaseInsensitiveCompare) instead.]
Comment 59 Rimas Kudelis 2010-10-05 13:13:20 PDT
could anyone answer my question from comment #57?
Comment 60 Mark Banner (:standard8, limited time in Dec) 2010-10-06 05:58:27 PDT
(In reply to comment #57)
> I can only see vCardImportAddressBadParam in the language files in the patch.
> Is that string unused?

Sorry for the delay, now fixed with rs from Neil over irc:

http://hg.mozilla.org/comm-central/rev/36440349f946

(In reply to comment #58)
> Comment on attachment 475533 [details] [diff] [review]
> Fixed up
> 
> [The commit date for this patch was 1 Apr, is this some kind of joke?]

That was probably something in the patch file to do with when it was last generated. Not sure why it got pushed through.

> >+  if (!line.EqualsIgnoreCase("BEGIN:VCARD")) {
> [EqualsIgnoreCase doesn't exist in the external API; I can't remember but maybe
>  we could use Equals("BEGIN:VCARD", CaseInsensitiveCompare) instead.]

Also fixed in the above changeset.
Comment 61 Rimas Kudelis 2010-10-06 06:02:49 PDT
(In reply to comment #60)
> (In reply to comment #57)
> > I can only see vCardImportAddressBadParam in the language files in the patch.
> > Is that string unused?
> 
> Sorry for the delay, now fixed with rs from Neil over irc:
> 
> http://hg.mozilla.org/comm-central/rev/36440349f946

Thanks. Though SeaMonkey is in string freeze ATM, so you'll want to post to m.d.l10n.
Comment 62 Mark Banner (:standard8, limited time in Dec) 2010-10-06 06:09:51 PDT
(In reply to comment #61)
> (In reply to comment #60)
> > (In reply to comment #57)
> > > I can only see vCardImportAddressBadParam in the language files in the patch.
> > > Is that string unused?
> > 
> > Sorry for the delay, now fixed with rs from Neil over irc:
> > 
> > http://hg.mozilla.org/comm-central/rev/36440349f946
> 
> Thanks. Though SeaMonkey is in string freeze ATM, so you'll want to post to
> m.d.l10n.

I've just backed out the change to the SeaMonkey file as its simpler for me. Someone else can re-land it later.
Comment 63 Rimas Kudelis 2010-10-06 06:38:19 PDT
OK, I filed bug 602188 about that.
Comment 64 Ludovic Hirlimann [:Usul] 2010-10-11 00:17:05 PDT
Tests 13624 and 13625 added to litmus.
Comment 65 Nikolay Shopik 2011-02-01 10:39:17 PST
*** Bug 630596 has been marked as a duplicate of this bug. ***
Comment 66 Mark Banner (:standard8, limited time in Dec) 2012-08-20 08:44:24 PDT
With bug 600798 landing the tests, this is now in-testsuite, thanks Evan.

Note You need to log in before you can comment on or make changes to this bug.