86089 - need to clean up mime attachments code

Status: VERIFIED FIXED

(MailNews Core :: MIME, defect, P2)

Description

[varada]

need to clean up mime attachments code- we use the same name as the forwarding 
subject for the temp file name and this forces us to hash the names.
we also need to use nsISupports instead of merely passing the attachments as 
strings.

Comment 1

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

spun off of bug #75449

Comment 2

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

as I understand it:

part of the problem is that when we forward a message, we create a temp file
using the subject of the message for the name.

it seems like we should be able to make the tmp file have one name, and the
attachment file name (within MIME) be something else.

Comment 3

[Jean-Francois Ducarroz]

I'll use this bug for the rewrite of the way we handle attachment in
nsIMsgCompFields and therefore inside mime as well.

In fact, we need to carry the following information for each attachment:
attachment name
url
content-type
charset
content-base
temporary, need to be deleted

I hope I did not forget anything...

Comment 4

[Jean-Francois Ducarroz]

*** Bug 88855 has been marked as a duplicate of this bug. ***

Comment 5

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v1

Comment 6

[Jean-Francois Ducarroz]

Attachment: and, there is the new file I forget to include in the first patch.

Comment 7

[Jean-Francois Ducarroz]

Comment on attachment 54825 [details] [diff] [review]
and, there is the new file I forget to include in the first patch.

R=varada

Comment 8

[Jean-Francois Ducarroz]

Comment on attachment 54824 [details] [diff] [review]
Proposed fix, v1

r=varada;about time we had the attachment array :)

Comment 9

[David :Bienvenu]

JF, could you fix the tabs in these files before you checkin? 

Comment 10

[Jean-Francois Ducarroz]

sure...

Comment 11

[Jean-Francois Ducarroz]

Attachment: proposed fix, v2 (same as v1 but untabified and updated with new tree)

Comment 12

[Jean-Francois Ducarroz]

Attachment: Changes explanation

Comment 13

[varada]

Comment on attachment 55275 [details] [diff] [review]
proposed fix, v2 (same as v1 but untabified and updated with new tree)

r=varada

Comment 14

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

Attachment: Changes explanation, as .txt

Comment 15

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

1) idl comments, like in nsIMsgAttachment

instead of //, do javadoc style comments.

so that they can be digestable by doxygen.

http://unstable.elemental.com/mozilla/

(see #16 on http://www.mozilla.org/hacking/reviewers.html)

good job on commenting the obsolete interfaces.  make sure to log bugs to remove
the obsolete stuff.

2)

I'm glad you fixed this:

-#ifdef XP_WIN
-  if (tempName && PL_strchr(tempName, '|'))
-  {
-    PR_Free(tempName);
-    tempName = nsnull;
-  }
-#endif
-#ifdef XP_MAC
-  if (tempName && PL_strlen(tempName) >= 31)
-    PR_DELETE(tempName)
-#endif

there's a bug on that somewhere, perhaps assigned to you, nhotta or me.

3)

fix string foo:

+          decodedSubject.InsertWithConversion("[Fwd: ", 0);
+          decodedSubject.AppendWithConversion("]");

instead do something like this:

decodedSubject.Insert(NS_LITERAL_STRING("Fwd: ").get(), 0);
decodedSubject.Append(NS_LITERAL_STRING("]").get());

other than that, it looks good.  some great cleanup, ducarroz.

sr=sspitzer

Comment 16

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

Comment on attachment 55275 [details] [diff] [review]
proposed fix, v2 (same as v1 but untabified and updated with new tree)

sr=sspitzer, once you clean up the idl comments and the string foo.

Comment 17

[Jean-Francois Ducarroz]

Answers to Seth question/comment:

1) will do before checked in
2) bug 89090
3) will do before check in

Thanks for the review.

Comment 18

[Jean-Francois Ducarroz]

fixed and checked in

Comment 19

[[e-mail] (gone - use [e-mail] instead)]

As I understand it, this bug was a re-organization, cleanup of code, and
therefore isn't limited to a single testcase to verify.  Verified that this was
checked into the trunk, and new bugs on individual bugs should be logged separately.