Last Comment Bug 621264 - the preference "mailnews.reply_header_originalmessage" in about:config is ignored for forwarded messages
: the preference "mailnews.reply_header_originalmessage" in about:config is ign...
Status: RESOLVED FIXED
[testday-20110603]
:
Product: MailNews Core
Classification: Components
Component: MIME (show other bugs)
: Trunk
: All All
-- normal (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
:
Mentors:
http://blog.chinaunix.net/photo/98822...
: 224811 (view as bug list)
Depends on: 70842 776438
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-23 22:37 PST by bo liang
Modified: 2012-07-22 23:24 PDT (History)
12 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Screen capturing (52.33 KB, image/png)
2010-12-23 22:39 PST, bo liang
no flags Details
patch proposal (4.21 KB, patch)
2012-02-12 08:23 PST, :aceman
neil: feedback+
Details | Diff | Splinter Review
patch v2 (4.64 KB, patch)
2012-02-26 09:58 PST, :aceman
no flags Details | Diff | Splinter Review
patch v3 (4.64 KB, patch)
2012-02-27 15:15 PST, :aceman
neil: review-
squibblyflabbetydoo: feedback+
Details | Diff | Splinter Review
patch v4 (4.69 KB, patch)
2012-03-30 12:22 PDT, :aceman
neil: review+
Details | Diff | Splinter Review
patch v5 (4.55 KB, patch)
2012-03-30 14:41 PDT, :aceman
neil: review+
Details | Diff | Splinter Review

Description User image bo liang 2010-12-23 22:37:37 PST
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 User image bo liang 2010-12-23 22:39:51 PST
Created attachment 499630 [details]
Screen capturing

Screen capturing
Comment 2 User image Ludovic Hirlimann [:Usul] 2011-01-03 04:33:31 PST
Does it changes if you restart Thunderbird ?
Comment 3 User image bo liang 2011-01-03 11:21:38 PST
(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 User image :aceman 2011-06-03 12:33:43 PDT
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 User image :aceman 2012-01-30 12:59:55 PST
Wayne, WADA, does this work for any of you?
Comment 6 User image :aceman 2012-02-01 05:26:05 PST
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 User image :aceman 2012-02-12 07:12:06 PST
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 User image :aceman 2012-02-12 08:23:57 PST
Created attachment 596486 [details] [diff] [review]
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 User image Jim Porter (:squib) 2012-02-16 00:26:06 PST
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 User image Ludovic Hirlimann [:Usul] 2012-02-16 00:27:49 PST
aceman for composition bienvenu or Ian.
Comment 11 User image :aceman 2012-02-16 02:57:15 PST
Comment on attachment 596486 [details] [diff] [review]
patch proposal

U can't use proper string types, as I have written:(
Comment 12 User image Ian Neal 2012-02-17 05:24:04 PST
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 User image David :Bienvenu 2012-02-17 13:46:12 PST
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 User image Jim Porter (:squib) 2012-02-17 13:58:18 PST
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 User image neil@parkwaycc.co.uk 2012-02-17 14:09:49 PST
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 User image :aceman 2012-02-17 14:37:29 PST
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 User image :aceman 2012-02-17 16:01:59 PST
Neil, so why is squib seeing leaks?
Comment 18 User image Jim Porter (:squib) 2012-02-17 16:18:17 PST
Actually, I don't think it does leak, but it is weird.
Comment 19 User image :aceman 2012-02-26 09:58:28 PST
Created attachment 600786 [details] [diff] [review]
patch v2

Thanks for the hints guys.
What about this?
Comment 20 User image neil@parkwaycc.co.uk 2012-02-26 10:40:43 PST
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 User image :aceman 2012-02-26 10:52:54 PST
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 User image :aceman 2012-02-27 15:15:41 PST
Created attachment 601085 [details] [diff] [review]
patch v3

Changing Assign to Adopt didn't work.
So let's try CopyUTF8 too there.
Comment 23 User image neil@parkwaycc.co.uk 2012-03-07 13:51:04 PST
(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 User image Jim Porter (:squib) 2012-03-18 00:33:10 PDT
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 User image :aceman 2012-03-18 05:59:03 PDT
Yes, see comment 7.
Comment 26 User image neil@parkwaycc.co.uk 2012-03-30 07:55:46 PDT
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 User image :aceman 2012-03-30 12:22:41 PDT
Created attachment 610971 [details] [diff] [review]
patch v4
Comment 28 User image neil@parkwaycc.co.uk 2012-03-30 12:51:39 PDT
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 User image :aceman 2012-03-30 14:41:31 PDT
Created attachment 611044 [details] [diff] [review]
patch v5

I wonder how many iterations we can get on such a short patch :)
Comment 30 User image Ryan VanderMeulen [:RyanVM] 2012-03-30 17:27:28 PDT
http://hg.mozilla.org/comm-central/rev/573b4c79043b
Comment 31 User image Mitar 2012-07-22 17:14:17 PDT
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 User image Mitar 2012-07-22 17:16:27 PDT
There was a similar bug 224811.
Comment 33 User image :aceman 2012-07-22 21:48:59 PDT
*** Bug 224811 has been marked as a duplicate of this bug. ***
Comment 34 User image :aceman 2012-07-22 21:53:01 PDT
That seems reasonable. Could you file a new bug with your request, blocking this one?
Comment 35 User image Mitar 2012-07-22 23:24:42 PDT
Done.

Note You need to log in before you can comment on or make changes to this bug.