371542 - Tidy up nsIImportABDescriptor.idl and associated code

Status: RESOLVED FIXED

(MailNews Core :: Import, defect)

Description

[Mark Banner (:standard8) (afk until 21st July)]

Attachment: The fix (diff -w)

I've been working on reducing duplicated import code (bug 131040). ReportError (used in various locations) would be a lot easier to tidy if GetPreferredName and SetPreferredName were just an attribute of type nsAString. We'd also avoid some extra string copies, allocs and frees.

As part of doing that I thought I may as well switch the interface to nsIFile while I was there. Although that means some extra translation code, it will start to help bug 323211.

I'm attaching -w and non -w versions. Neil would you also be able to check if this compiles on Windows for me? (I'll find someone to do mac before check-in)

Comment 1

[Mark Banner (:standard8) (afk until 21st July)]

Attachment: The fix (full patch)

Comment 2

[Stefan [:stefanh]]

Attachment: build failure

I applied the patch on my suiterunner tree and did a "make" in objdir/mailnews. It appears that the build fails.

Mac OS 10.3.9, using the 10.3.0 sdk with gcc3.3.

Comment 3

[Mark Banner (:standard8) (afk until 21st July)]

Attachment: The fix v2 (diff -w)

Fixes mac build problems as found by Stefan. Thanks to Stefan for helping with this.

Comment 4

[Mark Banner (:standard8) (afk until 21st July)]

Attachment: The fix v2 (full patch)

Comment 5

[[e-mail]]

Comment on attachment 256369 [details] [diff] [review]
The fix v2 (diff -w)

>-	inFile->GetNativePath( &pPath);    
>+	inFileSpec->GetNativePath(&pPath);
Might as well use the nsIFile here; saves converting again later.

>+    rv = NS_FileSpecToIFile(fileSpec, getter_AddRefs(fileLoc));
Should be &fileSpace here.

>-#include "nscore.h"
> #include "nsImportABDescriptor.h"
>+#include "nscore.h"
Any reason for this?

>+  /* readonly attribute nsIFile fileSpec; */
>+	NS_IMETHOD GetAbFile(nsIFile **aFile) {
>+    return mFile->Clone(aFile);
>+  }
>+
>+  NS_IMETHOD SetAbFile(nsIFile *aFile) {
>+    return aFile->Clone(getter_AddRefs(mFile));
>+  }
These need to be null-checked. I don't know how you want to deal with null files, you've got a choice of returning an error if you don't give a file to set or have a file to set, or returning null or clearing the internal file.

r=me with these fixed.

Comment 6

[Mark Banner (:standard8) (afk until 21st July)]

Attachment: The fix v3 (diff -w)

Fixes Neil's nits, carrying forward his r and requesting sr.

Comment 7

[Mark Banner (:standard8) (afk until 21st July)]

Attachment: The fix v3 (full patch)

Comment 8

[David :Bienvenu]

Comment on attachment 256924 [details] [diff] [review]
The fix v3 (diff -w)

thx, Mark!

Comment 9

[Mark Banner (:standard8) (afk until 21st July)]

Patch checked in -> fixed (including missed nit)