132180 - eliminate nsFileSpec in address book.

Status: VERIFIED FIXED

(MailNews Core :: Address Book, defect)

Description

[(not reading, please use [e-mail] instead)]

switch from nsFileSpec to nsIFile in nsIAddrBookSession.idl

nsFileSpec is deprecated.

Comment 1

[Mark Banner (:standard8)]

Attachment: Patch to switch nsFileSpec to nsIFile in nsIAddrBookSession.idl

This patch changes nsFileSpec to appropriate non-obsolete classes, for
nsIAddrBookSession.idl, note: although it touches many files, within those
files it only changes the necessary parts to run from the new nsIFile and
nsILocalFile classes for nsIAddrBookSession.idl, this way the patches can be
small and easy to review.

Note: I intend on morphing this bug to change nsFileSpec to nsIFile and friends
 for all of the address book section, but I am waiting to find out if dwitte is
working on it or not.

Comment 2

[Mark Banner (:standard8)]

Having spoken to dwitte, morphing this bug into a remove nsFileSpec from address
book, also taking bug and ccing seth.

Old title: switch from nsFileSpec to nsIFile in nsIAddrBookSession.idl
Changed OS to all as this isn't specific.

Comment 3

[Jungshik Shin]

There may be some problems for non-ASCII filenames (if they're used), which may
have to be dealt with in a separate bug. In 2, you assumed that |defaultName|
(char *) is in UTF-8 while elsewhere |char *| is assumed to be in the native
file system encoding. If they're guaranteed to be always in ASCII, there's no
problem (except that NS_ConvertUTF8toUTF16 is an overkill in that case), but I'm
not whether that's the case or not.
 
1. 
+                nsCAutoString fileName(mDirServerInfo->replInfo->fileName);
+                rv = dbPath->AppendNative(fileName);

2.

 /* This will generate a correct filename and then remove the path */
 void DIR_SetFileName(char** fileName, const char* defaultName)
.......

+		rv = dbPath->Append(NS_ConvertUTF8toUTF16(defaultName));

3. 
   nsCAutoString file(aURI + kMDBDirectoryRootLen);
   PRInt32 pos = file.Find("/");
   if (pos != kNotFound)
     file.Truncate(pos);
-  (*dbPath) += file.get();
+  rv = dbPath->AppendNative(file);
+  NS_ENSURE_SUCCESS(rv,rv);

Comment 4

[Mark Banner (:standard8)]

Attachment: Patch 1 v2

Revised initial partial patch to incorporate comments received and requesting
review from dmose.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 171789 [details] [diff] [review]
Patch 1 v2

Cancelling review on this version as the patch currently bitrots.

Comment 6

[Mark Banner (:standard8)]

Attachment: Patch 1 v3

Revised patch that fixes the bitrot - requesting review again.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 175061 [details] [diff] [review]
Patch 1 v3

In general, it's looking good; just a few nits...

>Index: mailnews/addrbook/public/nsIAddrBookSession.idl
>
> [...]
>
>-  [noscript] readonly attribute nsFileSpec userProfileDirectory;
>+  [noscript] readonly attribute nsILocalFile userProfileDirectory;

Why does this still need to be [noscript], now that nsFileSpec is gone?

>Index: mailnews/addrbook/public/nsIAddrDatabase.idl
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/addrbook/public/nsIAddrDatabase.idl,v
>retrieving revision 1.40
>diff -u -u -8 -p -r1.40 nsIAddrDatabase.idl
>--- mailnews/addrbook/public/nsIAddrDatabase.idl	20 Apr 2004 15:59:14 -0000	1.40
>+++ mailnews/addrbook/public/nsIAddrDatabase.idl	19 Jan 2005 20:29:43 -0000
>
> [...]
>
>-	[noscript] void open(in nsFileSpec folderName, in boolean create,
>+	[noscript] void open(in nsIFile folderName, in boolean create,
> 						out nsIAddrDatabase pCardDB, in boolean upgrading);
>-  nsIAddrDatabase openWithIFile(in nsIFile dbFile, in boolean create, in boolean upgrading);

Isn't the right thing to do here really drop the old open method
entirely, and just rename openWithIFile to open (thus retaining
scriptability as well as the method signature which is more useful
from JS and more readable in general).

>o +NS_IMETHODIMP nsAddrDatabase::Open
> +(nsIFile *aMabFile, PRBool aCreate, nsIAddrDatabase** pAddrDB, PRBool upgrading /* unused */)
> +{ 
> +  *pAddrDB = nsnull;
> +  
> +  nsCOMPtr<nsIFileSpec> aMabIFileSpec;
> +  nsFileSpec aMabFileSpec;

Please name this variable just mabFileSpec.  The "a" is a notation for
variables that were passed in via the arglist.

Please add commentary to the top of DIR_SetFileName explaining the
character set encoding assumptions about the parameters.

> @@ -824,22 +826,24 @@ nsIAddrDatabase *GetAddressBook( const P
> 			 * will proxy the observers because asynchronous directory
> 			 * implementations, such as LDAP, will assert results from
> 			 * a thread other than the UI thread.
> 			 *
> 			 */
> 			rv = proxyMgr->GetProxyForObject( NS_UI_THREAD_EVENTQ, NS_GET_IID( nsIAbDirectory),
> 				parentResource, PROXY_SYNC | PROXY_ALWAYS, getter_AddRefs( parentDir));
> 			if (parentDir)
>-		       	{
>+			{
> 				nsCAutoString URI("moz-abmdbdirectory://");
>-				URI.Append((*dbPath).GetLeafName());
>-				parentDir->CreateDirectoryByURI(name, URI.get (), PR_FALSE);
>-
>-          delete dbPath;
>+				nsCAutoString leafName;
>+				rv = dbPath->GetNativeLeafName(leafName);
>+				if (NS_SUCCEEDED(rv)) {
>+				  URI.Append(leafName);
>+				  parentDir->CreateDirectoryByURI(name, URI.get (), PR_FALSE);
>+				}
> 			}
> 
> 			IMPORT_LOG0( "Added new address book to the UI\n");
> 		}
> 	}
> 	
> 	return( pDatabase);
> }

It looks to me like the IMPORT_LOG0 could claim success here in case
of failure by GetNativeLeafName as well as CreateDirectorybyURI.
Yeah, I know that one of these problems was here before you got here,
but as long as you're here....

Comment 8

[Mark Banner (:standard8)]

Attachment: Patch 1 v4

This patch addresses dmose's comments, I have also added in some removal of
includes that are now redundant that I missed before.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 175652 [details] [diff] [review]
Patch 1 v4

Just a few tiny tweaks left:

>Index: mailnews/addrbook/public/nsIAddrBookSession.idl
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/addrbook/public/nsIAddrBookSession.idl,v
>retrieving revision 1.12
>diff -u -u -8 -p -r1.12 nsIAddrBookSession.idl
>--- mailnews/addrbook/public/nsIAddrBookSession.idl	17 Apr 2004 18:32:13 -0000	1.12
>+++ mailnews/addrbook/public/nsIAddrBookSession.idl	26 Feb 2005 16:30:26 -0000
>@@ -45,18 +45,19 @@
>  * than trying to port over the old MSG_Master in its entirety as that had a
>  * lot of cruft in it....
>  */
> 
> #include "nsISupports.idl"
> #include "nsIAbListener.idl"
> #include "nsIAbDirectory.idl"
> #include "nsIAbCard.idl"
>+#include "nsILocalFile.idl"

I think you should be able to get away with just forward-declaring the
nsILocalFile interface rather than including the idl, which will avoid slowing
down the build a bit.

>@@ -824,25 +826,35 @@ nsIAddrDatabase *GetAddressBook( const P
> 			 * will proxy the observers because asynchronous directory
> 			 * implementations, such as LDAP, will assert results from
> 			 * a thread other than the UI thread.
> 			 *
> 			 */
> 			rv = proxyMgr->GetProxyForObject( NS_UI_THREAD_EVENTQ, NS_GET_IID( nsIAbDirectory),
> 				parentResource, PROXY_SYNC | PROXY_ALWAYS, getter_AddRefs( parentDir));
> 			if (parentDir)
>-		       	{
>+			{
> 				nsCAutoString URI("moz-abmdbdirectory://");
>-				URI.Append((*dbPath).GetLeafName());
>-				parentDir->CreateDirectoryByURI(name, URI.get (), PR_FALSE);
>-
>-          delete dbPath;
>+				nsCAutoString leafName;
>+				rv = dbPath->GetNativeLeafName(leafName);
>+				if (NS_FAILED(rv)) {
>+				  IMPORT_LOG0( "*** Error: Unable to name of database\n");

The above line looks like it needs a verb.

>+				}
>+				else {
>+				  URI.Append(leafName);
>+				  rv = parentDir->CreateDirectoryByURI(name, URI.get (), PR_FALSE);
>+				  if (NS_FAILED(rv))
>+				    IMPORT_LOG0( "*** Error: Unable to create address book directory\n");
>+				}
> 			}
> 
>-			IMPORT_LOG0( "Added new address book to the UI\n");
>+			if (NS_SUCCEEDED(rv))
>+			  IMPORT_LOG0( "Added new address book to the UI\n");
>+			else
>+			  IMPORT_LOG0( "*** Error: An error occured whilst adding the address book to the UI\n");

I think "occurred" is the correct spelling, and I'd vote for switching to
"while", as "whilst" sounds a bit stilted.

Comment 10

[Mark Banner (:standard8)]

Attachment: Patch 1 v5

Fix dmose's comments and my bad english ;-)

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 175975 [details] [diff] [review]
Patch 1 v5

r=dmose

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 175975 [details] [diff] [review]
Patch 1 v5

Cancelling request for sr on this patch - I think it will break the palm sync
extension on windows, will investigate soon.

Comment 13

[Mark Banner (:standard8)]

Attachment: Patch 1 v6 (checked in)

Updated patch to fix bitrot and to include changes to nsAbPalmSync.cpp.

Dmose: although I said seperate patches on IRC, it was easier to just do the
one here - the only new file is nsAbPalmSync.cpp, the rest is the same as
before.

Also requesting sr at the same time.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 178275 [details] [diff] [review]
Patch 1 v6 (checked in)

r=dmose

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 178275 [details] [diff] [review]
Patch 1 v6 (checked in)

Biesi noted on check in that nsAddrBookSession::GetUserProfileDirectory could
have been reworked with a call to clone. I'll have a look at it for the next
patch.

Comment 17

[Mark Banner (:standard8)]

Attachment: Patch 2 v1

The main part of this patch is the removal of nsFileSpec from nsIAddrDatabase &
nsAddrDatabase, most of the rest is extra includes because of the removal of
the nsFileSpec.h include. I've tested all the main parts that I can and have
found no problems yet.

The Palm Sync part I haven't tested, but will ask David to - I'm fairly
confident it should work, I'm just not 100% sure if we'll get away with the
header files compiling with xpcom_obsolete removed from that extension. I'll
post back when I hear.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 179097 [details] [diff] [review]
Patch 2 v1

Sorry for the premature review request. David tested this and it needs some
more changes. It's too early to remove xpcom_obsolete from palmsync (mailnews
dependencies) :-( and there are some fixes to go into outlook items.

New patch coming in the next day or so.

Comment 19

[Mark Banner (:standard8)]

Attachment: Patch 2 v2

This version has been compiled by David (on windows) and he has added some
extra includes to outlook items that were not compiling. (Thanks david).

I've also dropped removing xpcom_obsolete from palmsync as items it depends on
within mailnews and so still needs it.

Most of the patch is now includes updating, the nsAddrDatabase & palm sync
contain the main changes.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 179485 [details] [diff] [review]
Patch 2 v2

Sorry for the delay reviewing this.  It looks pretty good, I've noted a few
minor issues:

>Index: mailnews/addrbook/src/nsAddrDatabase.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/addrbook/src/nsAddrDatabase.cpp,v
>retrieving revision 1.128
>diff -u -u -8 -p -r1.128 nsAddrDatabase.cpp
>--- mailnews/addrbook/src/nsAddrDatabase.cpp	23 Mar 2005 18:05:48 -0000	1.128
>+++ mailnews/addrbook/src/nsAddrDatabase.cpp	3 Apr 2005 18:19:35 -0000
>
> [...]
>   
>   // try one more time
>   // but first rename corrupt mab file
>   // and prompt the user
>   if (aCreate) 
>   {
>-    nsFileSpec *newMabFile = new nsFileSpec(mabFileSpec);
>-    if (!newMabFile)
>-      return NS_ERROR_OUT_OF_MEMORY;
>-    
>     // save off the name of the corrupt mab file, example abook.mab
>-    nsXPIDLCString originalMabFileName;
>-    originalMabFileName.Adopt(mabFileSpec.GetLeafName());
>-
>-    // the suggest new name for the backup will be abook.mab.bak
>-    nsCAutoString backupMabFileName(originalMabFileName);
>-    backupMabFileName += ".bak";
>+    nsCAutoString originalMabFileName;
>+    rv = aMabFile->GetNativeLeafName(originalMabFileName);
>+    NS_ENSURE_SUCCESS(rv, rv);
> 
>     // get a unique file name, using the suggested name
>     // if abook.mab.bak exists, abook.mab-1.bak will come back
>     // store that name
>-    newMabFile->MakeUnique(backupMabFileName.get());
>-    backupMabFileName.Adopt(newMabFile->GetLeafName());
>+    nsCOMPtr<nsIFile> newMabFile;
>+    rv = aMabFile->Clone(getter_AddRefs(newMabFile));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // the new name for the backup will be abook.mab.bak
>+    rv = aMabFile->AppendNative(NS_LITERAL_CSTRING(".bak"));
>+    NS_ENSURE_SUCCESS(rv, rv);

Shouldn't the above line be calling AppendNative on newMabFile, not aMabFile?

Also, it'd be nice to have a little commentary here explaining how the file
objects are used.  That is, using newMabFile to create a file and filename, and
then giving that to aMabFile, and then resetting the filename on newMabFile is
a pretty strange pattern.  In fact, it might be better to just add another
nsIFile object here called something like dummyBackupFile.  I suspect that
would make the code significantly easier to read.  Thoughts?

>@@ -3615,38 +3535,38 @@ NS_IMETHODIMP nsAddrDatabase::AddListDir
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>   static NS_DEFINE_CID(kRDFServiceCID, NS_RDFSERVICE_CID);
>     NS_WITH_PROXIED_SERVICE(nsIRDFService, rdfService, kRDFServiceCID, NS_UI_THREAD_EVENTQ, &rv);
>     if (NS_SUCCEEDED(rv)) 
>     {
>         nsCOMPtr<nsIRDFResource>    parentResource;
> 
>-        char* file = m_dbName.GetLeafName();
>-        char *parentUri = PR_smprintf("%s%s", kMDBDirectoryRoot, file);
>-        rv = rdfService->GetResource(nsDependentCString(parentUri), getter_AddRefs(parentResource));
>+        nsCAutoString fileName;
>+        rv = m_dbName->GetNativeLeafName(fileName);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+
>+        fileName.Insert(kMDBDirectoryRoot, 0);
>+
>+        rv = rdfService->GetResource(fileName, getter_AddRefs(parentResource));

Seems like it would make more sense to name the variable called fileName here
parentUri instead.

Finally, can you make sure that all the lines you've changed have spaces
instead of tabs now?  It should be easy to figure out which lines those are
just by grepping through the patch file.

Thanks!

Comment 21

[Mark Banner (:standard8)]

Attachment: Patch 2 v3

Revised to address review comments. Also added change to remove now redundant
comment in nsIAddrDatabase.idl

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 181452 [details] [diff] [review]
Patch 2 v3

I thought the plan was to go with the version of the patch with the explicit
dummy variable that you showed me instead of this one.	I don't have a high
degree of confidence that the right thing is going to happen with this patch
since CreateUnique actually causes a create to happen, and then we're monkeying
around with the filename afterwards.

Comment 23

[Mark Banner (:standard8)]

From discussion on IRC, this is what I need to do to get patch 2 a r+

dmose     Standard8: i'd still vote for the first version of the patch
dmose     Standard8: i'm actually more worried about windows
dmose     Standard8: since Close is never called on the CreateUnique file
dmose     Standard8: and then we attempt to Move something on top of it
Standard8 dmose: ok - version1.txt, possibly adding call to Close & I'll see if
I can get it verified as working on windows.
dmose     Standard8: excellent, thanks

Comment 24

[Mark Banner (:standard8)]

Attachment: Patch 3 v1

I need to do some more research on the changes in Patch 2, however, to keep
things moving, and as the ldif changes have made this easier, here is one that
sorts out the 4x migration parts - we don't use that code for mozilla,
(nsIAbUpgrade = "commercial" builds only) though it was suggested we should
keep it as we or netscape may use it in the future, and the patches are fairly
simple. The todo will be fixed in the patch that does the ldif service.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 188576 [details] [diff] [review]
Patch 3 v1


>-nsresult nsAddressBook::Migrate4xAb(nsIFileSpec *aFileSpec, PRBool aMigrating, PRBool aStoreLocAsHome)
>+nsresult nsAddressBook::Migrate4xAb(nsIFile *aFile, PRBool aMigrating, PRBool aStoreLocAsHome)
> {
>-  NS_ENSURE_ARG_POINTER(aFileSpec);
>+  NS_ENSURE_ARG_POINTER(aFile);
> 
>   nsresult rv = NS_OK;
> 
>   // We are migrating 4.x profile
>   /* Get database file name */
>-  char *dbUri = nsnull;
>-  char *leafName = nsnull;
>-  if (aFileSpec) {
>-    aFileSpec->GetLeafName(&leafName);
>+  nsCAutoString dbUri(NS_LITERAL_CSTRING(kMDBDirectoryRoot));

I thought it was possible to initialize an nsCAutoString with a char *, which,
if correct, means that the NS_LITERAL_CSTRING part shouldn't be necessary.

Otherwise looks great.	r=dmose, with the above change, assuming my memory
isn't faulty.

Comment 26

[Mark Banner (:standard8)]

Attachment: Patch 3 v2

This patch fixes review nit from dmose. Carring forward dmose's r, requesting
sr.

Comment 27

[David :Bienvenu]

the only existing 4x ab upgrade component uses the old interface. I'd like to
avoid breaking this until after 1.1.  It's unclear if there will ever be a 4xab
upgrader that works with nsIFile's, so it's quite possible this code should just
be removed or at the very least, disabled until there is a 4.x ab upgrade
component that works with nsIFile. The existing component is closed source, so
we don't have much flexibility there, other than writing a wrapper component.

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 189069 [details] [diff] [review]
Patch 3 v2

Clearing sr request whilst we wait for 1.9 to branch. I'll pick this part up
again after that.

Comment 29

[David :Bienvenu]

I've applied the last patch you gave me, Mark. The problem with the rename of
corrupt DB's is not due to your patch, I don't think. I believe it's because
nsFileSpec.MakeUnique now uses nsLocalFile::CreateUnique. MakeUnique just gets a
name that's going to be unique, whereas CreateUnique actually creates the file.
The problem is that we can't rename the corrupt ab as the unique name because
the file now already exists. We could delete the unique file before renaming,
but that would defeat the spirit of the change to MakeUnique. Doug, any
suggestions? We're just trying to find a unique name for a file, and then rename
the original file as the unique name.

Comment 30

[Doug Turner (:dougt)]

>We're just trying to find a unique name for a file, and then rename the
original file as the unique name.

I think the only safe thing to do is create the unique name for the file by
opening up a out file stream -- which will actually create the file and
internally return a FILE*.  Then, the original you would read in and write to
the out file stream.

I am not sure there is an 'atomic move to unique name' function on all platforms.

Comment 31

[Mark Banner (:standard8)]

Attachment: Patch 2 v4

Back to patch 2. A bit of the earlier versions of this has already gone in (bug
287003 - some of nsAddrDatabase::Open) so I've taken that out. I've also taken
as much of the nsFileSpec includes and definitions out of the headers as
possible, hence the additions of quite a few includes to counter that. The main
focus of the patch though is still the nsAddrDatabase and its interface
changes.

BTW: this patch won't go in before 1.8 branches, I think its too close to risk
it at the moment.

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 191405 [details] [diff] [review]
Patch 2 v4

Just some minor tweaks; otherwise looks good.

Don't forget to add your contact info to the license boilerplate of the files
you've touched.

Itd be nice to add doxygen commentary for IDL methods and attrs that youve
touched, but if youd prefer to defer that to a future patch, thats ok too.

>Index: mailnews/addrbook/public/nsIAddrDatabase.idl
> [...]
>-	[noscript] attribute nsFileSpec dbPath;
>+  attribute nsIFile dbPath;
> 	nsIAddrDatabase open(in nsIFile dbFile, in boolean create, in boolean upgrading);
>              
> 	void close(in boolean forceCommit);
>-	[noscript] void openMDB(in nsFileSpec dbName, in boolean create);
>+  void openMDB(in nsIFile dbName, in boolean create);

It looks to me like the emacs modeline in this file is wrong, and that the
majority of the non-tabbed lines in this file use 4-space indenting, not
2-space.  If you could fix the modeline and make your changes use 4-space
indenting, that'd be great.

>Index: mailnews/addrbook/src/nsAddrDatabase.cpp
>===================================================================
> [...]
>
> NS_IMETHODIMP nsAddrDatabase::Open
> (nsIFile *aMabFile, PRBool aCreate, PRBool upgrading /* unused */, nsIAddrDatabase** pAddrDB)
> { 
> [...]
>-      mabFileName.Adopt(mabFileSpec.GetLeafName());
>+      rv = aMabFile->GetNativeLeafName(mabFileName);
>+      NS_ENSURE_SUCCESS(rv, rv);
>       AlertAboutLockedMabFile(NS_ConvertASCIItoUCS2(mabFileName).get());

Since we don't actually know whether all the characters in the leaf name are
ASCII, I think a better thing to do here would be to use GetLeafName and then
ditch the NS_Convert* entirely.

>@@ -3477,19 +3384,21 @@ nsresult nsAddrDatabase::CreateABListCar
> 
>     mdbOid outOid;
>     mdb_id rowID=0;
> 
>     if (listRow->GetOid(GetEnv(), &outOid) == NS_OK)
>         rowID = outOid.mOid_Id;
> 
>     char* listURI = nsnull;
>-    char* file = nsnull;
>-    file = m_dbName.GetLeafName();
>-    listURI = PR_smprintf("%s%s/MailList%ld", kMDBDirectoryRoot, file, rowID);
>+
>+    nsCAutoString fileName;
>+    rv = m_dbName->GetNativeLeafName(fileName);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    listURI = PR_smprintf("%s%s/MailList%ld", kMDBDirectoryRoot, fileName.get(), rowID);

URIs are supposed to be UTF8.  So this probably needs to be gotten as UCS2,
then converted to UTF8 before being passed to PR_smprintf.

The next two patch hunks also have the same problem (one with RDF, one with
PR_smprintf).

Comment 33

[Mark Banner (:standard8)]

Attachment: Patch 2 v5 (diff -w) (non -w checked in)

Addressed Dan's review comments. This is a diff -w to avoid the whitespace
changes in nsIAddrDatabase.idl (Dan agreed on IRC that we change all the 4
space indented functions to 2 space).

Comment 34

[Mark Banner (:standard8)]

Attachment: Patch 2 v5 (checked in)

Full patch, with whitespace changes.

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 193493 [details] [diff] [review]
Patch 2 v5 (diff -w) (non -w checked in)

r=dmose

Comment 36

[David :Bienvenu]

Comment on attachment 193493 [details] [diff] [review]
Patch 2 v5 (diff -w) (non -w checked in)

sr=bienvenu, except that the indentation in nsAddrDatabase.h doesn't look quite
right, even in the -u diffs - are there tabs in existing file?

Comment 37

[Mark Banner (:standard8)]

Comment on attachment 193494 [details] [diff] [review]
Patch 2 v5 (checked in)

I've just checked this second patch in and also fixed the review nit as agreed
with bienvenu on IRC (correct emacs mode line in file header).

Comment 38

[Mark Banner (:standard8)]

Attachment: Patch 4 v1 (checked in)

This patch removes all the file spec related code from nsVCard* in address book
- none of this is currently used by Thunderbird or SeaMonkey and when we do get
round to improving our vcard support, we'll probably want to rewrite it anyway.

Comment 39

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 194005 [details] [diff] [review]
Patch 4 v1 (checked in)

r=dmose

Comment 40

[David :Bienvenu]

Comment on attachment 194005 [details] [diff] [review]
Patch 4 v1 (checked in)

thx, Mark.

Comment 41

[Mark Banner (:standard8)]

Comment on attachment 194005 [details] [diff] [review]
Patch 4 v1 (checked in)

ok, this one is now in.

Comment 42

[Mark Banner (:standard8)]

(In reply to comment #27)
> the only existing 4x ab upgrade component uses the old interface. I'd like to
> avoid breaking this until after 1.1.  It's unclear if there will ever be a 4xab
> upgrader that works with nsIFile's, so it's quite possible this code should just
> be removed or at the very least, disabled until there is a 4.x ab upgrade
> component that works with nsIFile. The existing component is closed source, so
> we don't have much flexibility there, other than writing a wrapper component.

I have just tried the copying of nsAB4xUpgrader.dll from Netscape 6 to our
equivalent directory on SeaMonkey 1.0a and the address book did not migrate from
a 4.x profile (I got an na2 in my profile dir, but nothing else). Therefore I
guess this no longer works.

As previously discussed on IRC with Dan & David, as this method no longer works
we'll remove the interface pending possible work on a new 4.x ab upgrade interface.

Comment 43

[Mark Banner (:standard8)]

Attachment: Patch 3 v3

As discussed above & on IRC, this patch now completely removes the
nsIAbUpgrader interface for 4.x address books (which no longer works for us as
it is closed source) and its related file spec code. Hence requesting fresh
reviews for this patch.

Comment 44

[Mark Banner (:standard8)]

Comment on attachment 196427 [details] [diff] [review]
Patch 3 v3

I missed a file when formulating the patch, correct one coming up...

Comment 45

[Mark Banner (:standard8)]

Attachment: Patch 3 v4 (checked in)

Same as the previous (v3) patch, but with the missing
nsDogbertProfileMigrator.cpp change included.

Comment 46

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 196530 [details] [diff] [review]
Patch 3 v4 (checked in)

Is there any reason not to just remove the stuff that you commented out?  We've
got CVS history if we need to look at it or get it back for some reason.

Comment 47

[Mark Banner (:standard8)]

Comment on attachment 196530 [details] [diff] [review]
Patch 3 v4 (checked in)

Requesting sr, I'll remove the commented out na2 file copying when I check in
(as per Dan's review comment)

Comment 48

[Mark Banner (:standard8)]

Attachment: Patch 5 v1

This is the final patch (hopefully) that removes nsFileSpec from the
nsIAbLDIFService, and also xpcom_obsolete dependencies from mailnews/addrbook
and mailnews/extensions/palmsync.

I've had an earlier version of this patch tested on Mac, Windows & Linux so we
should hopefully be ok with regards to removing those deps. Note the only
changes from the earlier version where some algorithm ones to fix some bugs I'd
introduced.

Comment 49

[Dan Mosedale (:dmosedale, :dmose)]

Since there's one big chunk where a lot of whitespace cleanup happened, can you
attach a "diff -uw" for easier reviewing?  Thanks!

Comment 50

[Mark Banner (:standard8)]

Attachment: Patch 5 v1 (diff -w)

The -w version of patch 5.

Comment 51

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 198492 [details] [diff] [review]
Patch 5 v1 (diff -w)

> // Count total number of legal ldif fields and records in the first 100 lines of the 
> // file and if the average legal ldif field is 3 or higher than it's a valid ldif file.
>-NS_IMETHODIMP nsAbLDIFService::IsLDIFFile( nsIFileSpec *pSrc, PRBool *_retval)
>+NS_IMETHODIMP nsAbLDIFService::IsLDIFFile(nsIFile *pSrc, PRBool *_retval)
> {
>+  NS_ENSURE_ARG_POINTER(pSrc);
>+  NS_ENSURE_ARG_POINTER(_retval);
>+
>     *_retval = PR_FALSE;
> 
>-    nsresult rv = pSrc->OpenStreamForReading();
>+  nsresult rv = NS_OK;
>+
>+  nsCOMPtr<nsIFileInputStream> fileStream(do_CreateInstance(NS_LOCALFILEINPUTSTREAM_CONTRACTID, &rv));
>     NS_ENSURE_SUCCESS(rv, rv);

Any particular reason not to use NS_NewLocalFileInputStream here?

>     PRBool    wasTruncated = PR_FALSE;

This variable can go away now, right?

r=dmose

Comment 52

[Mark Banner (:standard8)]

Attachment: Patch 5 v2 (diff -w) (non -w checked in)

Address Dan's review comments, and add update for uuid in nsIAbLDIFService that
I forgot earlier.

Non -w patch for checkin coming in a moment.

Comment 53

[Mark Banner (:standard8)]

Attachment: Patch 5 v2 (checked in)

The one for checkin assuming the other one get review flags set ok.

Comment 54

[Mark Banner (:standard8)]

Now that the final patch is in the address book doesn't depend on
xpcom_obsolete. It still links against it but that's because other mailnews
modules use it and windows & mac get upset if you don't have it in the linker
list... but the rest of mailnews is another bug (bug 33451 in fact). However,
its good enough for this bug to be closed :-)

Thanks to dmose & bienvenu for all the help with getting this bug fixed.

Comment 55

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED via LXR and basic addressbook functionality testing using my
self-built Windows XP SeaMonkey trunk build.