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

RESOLVED FIXED in mozilla1.4beta

Status

MailNews Core
Composition
RESOLVED FIXED
15 years ago
9 years ago

People

(Reporter: Cavin Song, Assigned: Cavin Song)

Tracking

Trunk
mozilla1.4beta
PowerPC
Mac OS X

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

20.13 KB, patch
Cavin Song
: review+
Simon Fraser
: superreview+
(not reading, please use seth@sspitzer.org instead)
: approval1.4b+
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
The code is not turned on for OS X in mailnew/compose.
(Assignee)

Updated

15 years ago
Summary: ([mach-o] Can't send Apple single and double attachments → [mach-o] Can't send Apple single and double attachments
(Assignee)

Comment 1

15 years ago
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)
 
(Assignee)

Comment 2

15 years ago
let's start with:

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

Comment 3

15 years ago
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.
(Assignee)

Comment 4

15 years ago
Created attachment 121302 [details] [diff] [review]
Proposed patch, v1

Fix apple single/double sending and receiving problems.
(Assignee)

Comment 5

15 years ago
In nsMsgAppleCodes.h, I have to turn on:

#pragma options align=mac68k

to make encoding of resource folk work.
(Assignee)

Updated

15 years ago
Attachment #121302 - Flags: review?(ccarlen)
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;
Attachment #121302 - Flags: review?(ccarlen) → review+
(Assignee)

Comment 7

15 years ago
Created attachment 121465 [details] [diff] [review]
Proposed patch, v2

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

Updated

15 years ago
Attachment #121302 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #121465 - Flags: review?(ccarlen)
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
>         }
>
Attachment #121465 - Flags: review?(ccarlen) → review+
(Assignee)

Comment 9

15 years ago
>>+#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.
(Assignee)

Comment 10

15 years ago
Created attachment 121471 [details] [diff] [review]
Proposed patch, v3

Incorporated comments.
Attachment #121465 - Attachment is obsolete: true
(Assignee)

Comment 11

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

Carry over r=ccarlen.
Attachment #121471 - Flags: review+
(Assignee)

Updated

15 years ago
Attachment #121471 - Flags: superreview?(sfraser)
(Assignee)

Updated

15 years ago
Flags: blocking1.4b?

Comment 12

15 years ago
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).
Attachment #121471 - Flags: superreview?(sfraser) → superreview+
a=sspitzer for 1.4 beta
Target Milestone: --- → mozilla1.4beta
clearning the blocking flag, but go ahead and land
Flags: blocking1.4b?
Comment on attachment 121471 [details] [diff] [review]
Proposed patch, v3

a=sspitzer
Attachment #121471 - Flags: approval1.4b+
(Assignee)

Comment 16

15 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Updated

15 years ago
Hardware: PC → Macintosh
Summary: [mach-o] Can't send Apple single and double attachments → [mach-o] Can't send AppleSingle and AppleDouble attachments
+              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.
I mean the second line doesn't look right.
(Assignee)

Comment 19

15 years ago
Yeah, it's not right.
I've fixed that line in nsMessenger.cpp.
(Assignee)

Comment 21

15 years ago
Thanks Seth, good catch.
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.