202355 - [mach-o] Can't send AppleSingle and AppleDouble attachments

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Cavin Song]

The code is not turned on for OS X in mailnew/compose.

Comment 1

[Cavin Song]

not sure which of these we will get to, but here are some test cases:

send email from eudora 5.2
 binhex
 apple single
 apple double
 uuencode
 base64
send email from 4.x (CFM)
 mime
 uuencode
email from 7.02 (mozilla, CFM)

sending email from mach-o to:
  eudora
  4.x
  7.02
  7.1

when sending / receiving, we should try simple images and sending applications
(which are true apple double documents)
 

Comment 2

[Cavin Song]

let's start with:

1) receiving apple double (an application, like GraphingCalculator)
2) sending apple double (an application, like GraphingCalculator)

Comment 3

[Cavin Song]

Sending and receiving apple double files seems to work now. We don't seem to
send  files in apple single format but receiving apple single files from eudora
works fine.

Comment 4

[Cavin Song]

Attachment: Proposed patch, v1

Fix apple single/double sending and receiving problems.

Comment 5

[Cavin Song]

In nsMsgAppleCodes.h, I have to turn on:

#pragma options align=mac68k

to make encoding of resource folk work.

Comment 6

[Conrad Carlen (not reading bugmail)]

Comment on attachment 121302 [details] [diff] [review]
Proposed patch, v1

Just a few little things. With them, r=ccarlen

>Index: mailnews/compose/src/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/compose/src/Makefile.in,v
>retrieving revision 1.65
>diff -u -w -r1.65 Makefile.in
>--- mailnews/compose/src/Makefile.in	15 Mar 2003 01:03:16 -0000	1.65
>+++ mailnews/compose/src/Makefile.in	22 Apr 2003 18:20:17 -0000
>@@ -126,6 +126,17 @@
> 		nsSmtpDelegateFactory.h \
> 		$(NULL)
> 
>+# MacOSX requires the MoreFiles module
>+ifeq ($(OS_ARCH),Darwin)

According to cls, you should use the same toolkit test here:
>+ifneq (,$(filter cocoa mac, $(MOZ_WIDGET_TOOLKIT)))
as above instead of Darwin test.

>+REQUIRES += macmorefiles \
>+		$(NULL)
>+CPPSRCS += nsMsgAppleDoubleEncode.cpp \
>+		nsMsgAppleEncode.cpp \
>+		$(NULL)
>+EXPORTS += nsMsgAppleDouble.h \
>+		$(NULL)

>Index: mailnews/compose/src/nsMsgAppleDoubleEncode.cpp
>===================================================================
>@@ -64,7 +67,14 @@
> 	*encoding = NULL;
> 
> 	FInfo		fndrInfo;
>+#if defined(XP_MAC)
>   OSErr err = FSpGetFInfo( fs->GetFSSpecPtr(), &fndrInfo );
>+#else
>+  FSSpec fsSpec;
>+  FSPathMakeFSSpec((UInt8 *)fs->GetNativePathCString(), &fsSpec, NULL);
>+  OSErr err = FSpGetFInfo (&fsSpec, &fndrInfo);

Granted, this code is full of FSSpec usage, but try not to introduce any new
FSSpec usage into XP_MACOSX code if there's a way to do the same thing using
FSRefs. You can do the same thing here by:

FSPathMakeRef() followed by
FSGetFinderInfo().

>Index: mailnews/compose/src/nsMsgAttachmentHandler.cpp
>===================================================================
>+#if defined(XP_MAC) || defined(XP_MACOSX)
>+PRBool nsMsgAttachmentHandler::HasResourceFolk(FSSpec *fsSpec)

It's Fork with an 'r'

>Index: mailnews/compose/src/nsMsgAttachmentHandler.h
>===================================================================
>Index: mailnews/base/src/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/src/Makefile.in,v
>retrieving revision 1.103
>diff -u -w -r1.103 Makefile.in
>--- mailnews/base/src/Makefile.in	15 Mar 2003 01:03:13 -0000	1.103
>+++ mailnews/base/src/Makefile.in	22 Apr 2003 18:20:18 -0000
>@@ -108,6 +108,11 @@
> 		nsCidProtocolHandler.cpp \
> 		$(NULL)
> 
>+# MacOSX requires the MoreFiles module
>+ifeq ($(OS_ARCH),Darwin)

Use the toolkit test, like above.

>+REQUIRES += macmorefiles
>+endif

>Index: mailnews/base/src/nsMessenger.cpp
>===================================================================
>   nsresult rv = NS_OK;
>-#if defined(XP_MAC)
>+#if defined(XP_MAC)  /* reviewed for 1.4, XP_MACOSX not needed */

What do you mean here?

>   /* We need to truncate the name to 31 characters, this even on MacOS X until the file API
>      correctly support long file name. Using a nsILocalFile will do the trick...
>   */
>           if (NS_SUCCEEDED(mimeinfo->GetMacType(&aMacType)) && NS_SUCCEEDED(mimeinfo->GetMacCreator(&aMacCreator)))
>+#if defined(XP_MAC)
>             realSpec.SetFileTypeAndCreator((OSType)aMacType, (OSType)aMacCreator);
>+#else
>+          {
>+            nsCOMPtr<nsILocalFileMac> macFile =  do_QueryInterface(outputFile, &rv);
>+            if (NS_SUCCEEDED(rv) && macFile)
>+            {
>+              macFile->SetFileCreator((OSType)aMacCreator);
>+              macFile->SetFileCreator((OSType)aMacType);
>+            }
>+          }
>+#endif

XP_MAC and XP_MACOSX should be the same here - they both have nsILocalFileMac.
Not that we really care about XP_MAC, but no need to throw in additional
#ifdefs for it.

>         }
>       }
>       
>Index: mailnews/base/util/nsMsgUtils.cpp
>===================================================================
> nsresult NS_MsgHashIfNecessary(nsCAutoString &name)
> {
>-#if defined(XP_MAC)
>+#if defined(XP_MAC) || defined(XP_MACOSX)
>   const PRUint32 MAX_LEN = 25;

Why is XP_MACOSX being limited to this value? Are we using FSSpecs here too?

> #elif defined(XP_UNIX) || defined(XP_BEOS)
>   const PRUint32 MAX_LEN = 55;

Comment 7

[Cavin Song]

Attachment: Proposed patch, v2

Incorporate comment. Also, the changes to mailnews/mime/src were missing so
they are included here.

Comment 8

[Conrad Carlen (not reading bugmail)]

Comment on attachment 121465 [details] [diff] [review]
Proposed patch, v2

Just 2 things - fix those and r=ccarlen

>Index: mailnews/compose/src/nsMsgAttachmentHandler.cpp
>===================================================================

>+#if defined(XP_MAC) || defined(XP_MACOSX)
>+PRBool nsMsgAttachmentHandler::HasResourceFork(FSSpec *fsSpec)
>+{
>+#if defined(XP_MACOSX)
>+  FSRef fsRef;
>+  if (::FSpMakeFSRef(fsSpec, &fsRef) == noErr)
>+  {
>+    FSCatalogInfo catalogInfo;
>+    OSErr err = ::FSGetCatalogInfo(&fsRef, kFSCatInfoDataSizes + kFSCatInfoRsrcSizes, &catalogInfo, nsnull, nsnull, nsnull);
>+    return (err == noErr && catalogInfo.rsrcLogicalSize != 0);

You should be able to do the XP_MACOSX code here. Both FSpMakeFSRef and
FSGetCatalogInfo are OS routines that are available in either build.

>+  }
>+  return PR_FALSE;
>+#else
>+  long dataSize = 0;
>+  long resSize = 0;
>+  return (::FSpGetFileSize(&fsSpec, &dataSize, &resSize) == noErr && resSize != 0);
>+#endif
>+}
>+#endif
>+


>           // else don't need to use apple double.
>-          if (::FSpGetFileSize(&fsSpec, &dataSize, &resSize) == noErr && resSize == 0)
>-            sendResourceFork = PR_FALSE;
>+#if defined(XP_MAX) || defined(XP_MAXOSX)

Typo - XP_MAC, not XP_MAX (both places)

>+          sendResourceFork = HasResourceFolk(&fsSpec);
>+#endif
>         }
>

Comment 9

[Cavin Song]

>>+#if defined(XP_MAX) || defined(XP_MAXOSX)
>
>Typo - XP_MAC, not XP_MAX (both places)
>
>>+          sendResourceFork = HasResourceFolk(&fsSpec);
>
Good catch, for the tests I did it never went through the path so I did not
notice it.

Comment 10

[Cavin Song]

Attachment: Proposed patch, v3

Incorporated comments.

Comment 11

[Cavin Song]

Comment on attachment 121471 [details] [diff] [review]
Proposed patch, v3

Carry over r=ccarlen.

Comment 12

[Simon Fraser [no longer active]]

Comment on attachment 121471 [details] [diff] [review]
Proposed patch, v3

The XP_MAC stuff looks fine, but I'd like to see bugs filed on two other
issues:
i) fix the code that decides whether to AppleSingle to be smarter than just
sniffing for a resource fork
ii) Behave correctly when the user tries to attach a packaged application or
document (i.e. something that is really a directory).

Comment 13

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

a=sspitzer for 1.4 beta

Comment 14

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

clearning the blocking flag, but go ahead and land

Comment 15

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

Comment on attachment 121471 [details] [diff] [review]
Proposed patch, v3

a=sspitzer

Comment 16

[Cavin Song]

Fix checked in.

Comment 17

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

+              macFile->SetFileCreator((OSType)aMacCreator);
+              macFile->SetFileCreator((OSType)aMacType);

I'm copying some of this code for another bug, #182174

that code doesn't look right.

shouldn't it be SetFileType()?  I'll include it in my patch for the other bug.

Comment 18

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

I mean the second line doesn't look right.

Comment 19

[Cavin Song]

Yeah, it's not right.

Comment 20

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

I've fixed that line in nsMessenger.cpp.

Comment 21

[Cavin Song]

Thanks Seth, good catch.