621264 - the preference "mailnews.reply_header_originalmessage" in about:config is ignored for forwarded messages

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[bo liang]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.224 Safari/534.10
Build Identifier: 3.1.7

The problem be happened on windowsXP and Ubuntu.
About:config "mailnews.reply_header_originalmessage" is invalid!

please see the screen capturing:
http://blog.chinaunix.net/photo/98822_101224141637.png



Reproducible: Always

Steps to Reproduce:
1. change the value of "mailnews.reply_header_originalmessage" from "原始消息" to "Orginal Message"
2. Forward one mail

Actual Results:  
mail header text :"原始消息"

Expected Results:  
mail header text :"Orginal Message"

Thunderbird Setup 3.1.7.exe
Language:Chinese simplified

Comment 1

[bo liang]

Attachment: Screen capturing

Screen capturing

Comment 2

[Ludovic Hirlimann [:Usul]]

Does it changes if you restart Thunderbird ?

Comment 3

[bo liang]

(In reply to comment #2)
> Does it changes if you restart Thunderbird ?

I already restarted Thunderbird but "mailnews.reply_header_originalmessage" still invalided.
Thunderbird(linux) and Thunderbird(windows) is same.(invalided)

Comment 4

[:aceman]

I can confirm this on 3.1.10 english version. The value of the preferrence is not taken into account. The delimiter is always the fixed string "---Original message---".

Comment 5

[:aceman]

Wayne, WADA, does this work for any of you?

Comment 6

[:aceman]

This was added in Bug 70842.

This looks like the function what generates the message and should use the pref: http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#2234

I'll try to look why it does not work.

Comment 7

[:aceman]

The change in bug 70842 seems to be only from message reply.
The forward case is taken from a different spot in MIME mimedrft.cpp.
I am not sure we want to hook it on the same preference.
But I can try to prepare the infrastructure.

Comment 8

[:aceman]

Attachment: patch proposal

So I currently hooked onto the existing pref of mailnews.reply_header_originalmessage . I get its value in the same way as http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp . I am not yet familiar with these ns*String games, but these conversions worked for me.

Comment 9

[Jim Porter (:squib)]

Comment on attachment 596486 [details] [diff] [review]
patch proposal

I'm probably not the best person to look at this, since I don't know much about composition. In any case, this seems ok, assuming you update the patch to use the proper string types (it leaks memory now).

Comment 10

[Ludovic Hirlimann [:Usul]]

aceman for composition bienvenu or Ian.

Comment 11

[:aceman]

Comment on attachment 596486 [details] [diff] [review]
patch proposal

U can't use proper string types, as I have written:(

Comment 12

[Ian Neal]

Comment on attachment 596486 [details] [diff] [review]
patch proposal

I'm not strong enough on best way to do strings in C++, try David

Comment 13

[David :Bienvenu]

Comment on attachment 596486 [details] [diff] [review]
patch proposal

Neil's the expert on this string stuff...

but the static buffer for a return value is ugly - better to use an nsCString temp variable, and have MimeGetReplyHeaderOriginalMessage fill that in, IMO. And then do NS_MsgSACat(&newBody, tempString.get())

Comment 14

[Jim Porter (:squib)]

Here's a guide on string usage in Mozilla: https://developer.mozilla.org/En/Mozilla_internal_string_guide

As Bienvenu implies, I think you'll want to make your new function look like:

  nsresult MimeGetReplyHeaderOriginalMessage(nsAString &str) { ... }

Hopefully that will give you a nudge in the right direction. :)

Comment 15

[[e-mail]]

Comment on attachment 596486 [details] [diff] [review]
patch proposal

Well, this is par for the course for MIME; I can't spot any leaks, so f+...

>+  NS_GetLocalizedUnicharPreferenceWithDefault(nsnull, "mailnews.reply_header_originalmessage",  tmpString2, tmpString3);
Although I would have inlined the conversion something like this:
NS_GetLocalizedUnicharPreferenceWithDefault(nsnull,
    "mailnews.reply_header_originalmessage",
     NS_ConvertUTF8toUTF16(tmpString1), tmpString3);

Comment 16

[:aceman]

Thanks, I'll try to find out what you are talking about :)
The static return string is just copied from the previously used function (above mine) so as not to break expectations of the callers.

Comment 17

[:aceman]

Neil, so why is squib seeing leaks?

Comment 18

[Jim Porter (:squib)]

Actually, I don't think it does leak, but it is weird.

Comment 19

[:aceman]

Attachment: patch v2

Thanks for the hints guys.
What about this?

Comment 20

[[e-mail]]

Comment on attachment 600786 [details] [diff] [review]
patch v2

>+    NS_ConvertUTF8toUTF16(MimeGetStringByID(MIME_FORWARDED_MESSAGE_HTML_USER_WROTE)));
Where did the Adopt go? I believe it was needed to avoid a leak.

>+  retString.Assign(NS_ConvertUTF16toUTF8(tmpRetString));
Nit: Use CopyUTF16toUTF8 instead.

Comment 21

[:aceman]

It changed to Assign...

Adopt is not even mentioned on
https://developer.mozilla.org/En/Mozilla_internal_string_guide
so I don't know when it must be used (I used it for literals only).

Comment 22

[:aceman]

Attachment: patch v3

Changing Assign to Adopt didn't work.
So let's try CopyUTF8 too there.

Comment 23

[[e-mail]]

(In reply to aceman from comment #22)
> Changing Assign to Adopt didn't work.
Well, you can't assign or adopt directly into your destination because you have a UTF8/UTF16 mismatch, so you need to adopt into a temporary string.

I'm not sure where it's documented but so you know, Adopt is used when you have a string created through NS_Alloc/NS_Strdup/ToXXXString and you want your ns(C)String to automatically NS_Free it for you so that you don't leak it.

Comment 24

[Jim Porter (:squib)]

Comment on attachment 601085 [details] [diff] [review]
patch v3

Review of attachment 601085 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is ok, at least string-wise. I'm assuming this pref is used in some wholly-different part of the code for replies?

Comment 25

[:aceman]

Yes, see comment 7.

Comment 26

[[e-mail]]

Comment on attachment 601085 [details] [diff] [review]
patch v3

>+  CopyUTF8toUTF16(MimeGetStringByID(MIME_FORWARDED_MESSAGE_HTML_USER_WROTE), defaultValue);
Still waiting for you to adopt the return value of MimeGetStringByID...

Comment 27

[:aceman]

Attachment: patch v4

Comment 28

[[e-mail]]

Comment on attachment 610971 [details] [diff] [review]
patch v4

>+nsresult
You always return NS_OK, so you might as well make this void anyway.

>+  nsString defaultValue;
>+  CopyUTF8toUTF16(tmpString, defaultValue);
Actually this is better written as NS_ConvertUTF8toUTF16 defaultValue(tmpString); but see also below.

>+  if (NS_FAILED(rv))
>+    tmpRetString.Assign(defaultValue);
NS_GetLocalizedUnicharPreferenceWithDefault always returns NS_OK, so you don't need this (or indeed rv). (This also means that you only use defaultValue once, so you can inline it with NS_ConvertUTF8toUTF16(tmpString).)

>     NS_MsgSACopy(&(newBody), "<HTML><BODY><BR><BR>");
> 
>+
Nit: unnecessary duplicate blank line.

Comment 29

[:aceman]

Attachment: patch v5

I wonder how many iterations we can get on such a short patch :)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/573b4c79043b

Comment 31

[Mitar]

Aaa. Why is reply_header_originalmessage used also for forwarded messages? It should be used only for reply messages, as the name tells. With this patch I am now getting also forwarded messages formatted in the same way as replies. And this is not correct in my configuration.

I believe there should be a separate configuration for forwarded messages and that it was correct behavior that reply_header_originalmessage was not used for forwarded messages.

Comment 32

[Mitar]

There was a similar bug 224811.

Comment 34

[:aceman]

That seems reasonable. Could you file a new bug with your request, blocking this one?

Comment 35

[Mitar]

Done.