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

Screen capturing
Comment 2 Ludovic Hirlimann [:Usul] 2011-01-03 04:33:31 PST
Does it changes if you restart Thunderbird ?
Comment 3 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 :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 :aceman 2012-01-30 12:59:55 PST
Wayne, WADA, does this work for any of you?
Comment 6 :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 :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 :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 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 Ludovic Hirlimann [:Usul] 2012-02-16 00:27:49 PST
aceman for composition bienvenu or Ian.
Comment 11 :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 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 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 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 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 :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 :aceman 2012-02-17 16:01:59 PST
Neil, so why is squib seeing leaks?
Comment 18 Jim Porter (:squib) 2012-02-17 16:18:17 PST
Actually, I don't think it does leak, but it is weird.
Comment 19 :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 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 :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 :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 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 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 :aceman 2012-03-18 05:59:03 PDT
Yes, see comment 7.
Comment 26 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 :aceman 2012-03-30 12:22:41 PDT
Created attachment 610971 [details] [diff] [review]
patch v4
Comment 28 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 :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 Ryan VanderMeulen [:RyanVM] 2012-03-30 17:27:28 PDT
http://hg.mozilla.org/comm-central/rev/573b4c79043b
Comment 31 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 Mitar 2012-07-22 17:16:27 PDT
There was a similar bug 224811.
Comment 33 :aceman 2012-07-22 21:48:59 PDT
*** Bug 224811 has been marked as a duplicate of this bug. ***
Comment 34 :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 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.