Last Comment Bug 133605 - nsIMsgCompFields::[Set|Get]NewsHost is identical to nsIMsgCompFields::[Set|Get]NewspostUrl
: nsIMsgCompFields::[Set|Get]NewsHost is identical to nsIMsgCompFields::[Set|Ge...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Composition (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 19.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-03-26 14:22 PST by Jean-Francois Ducarroz
Modified: 2012-10-09 17:40 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (4.85 KB, patch)
2012-10-04 12:28 PDT, :aceman
rkent: review+
Details | Diff | Splinter Review
patch v2 (4.81 KB, patch)
2012-10-09 11:56 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description Jean-Francois Ducarroz 2002-03-26 14:22:31 PST
Those 2 set of APIs access the same header in the compose fields:
MSG_NEWSPOSTURL_HEADER_ID. Some time we use one and some time the other one. One
of them must go away...
Comment 1 Simon Paquet [:sipaq] 2003-08-14 17:36:53 PDT
not a mac classic bug
Comment 2 Wayne Mery (:wsmwk, NI for questions) 2011-02-02 04:24:52 PST
(still see this)
Joshua, do you see any reason to favor NewspostUrl over NewsHost or vice versa?
http://mxr.mozilla.org/comm-central/search?string=MSG_NEWSPOSTURL_HEADER_ID&find=nsMsgCompFields.cpp&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
Comment 3 Joshua Cranmer [:jcranmer] 2011-02-02 06:10:18 PST
We appear to universally use NewspostUrl instead of NewsHost, so I would consider that a good reason :-)
Comment 4 :aceman 2011-11-06 12:49:43 PST
This still seems relevant, should I try this?
Comment 5 :aceman 2012-10-04 12:28:47 PDT
Created attachment 668118 [details] [diff] [review]
patch

Please also decide what to do with the code that was 'if 0'ed.

Does this need a superreview?
Comment 6 Magnus Melin 2012-10-04 22:43:18 PDT
Comment on attachment 668118 [details] [diff] [review]
patch

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

As an interface change in theory yes, but given what it does, probably not :)
Not that i'm a superreviewer.
Comment 7 Magnus Melin 2012-10-05 00:55:48 PDT
Comment on attachment 668118 [details] [diff] [review]
patch

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

Not a mailnews/ reviewer either, but it looks ok to me. Dunno about the ifdef 0 change. Did you test if it works properly?
Comment 8 :aceman 2012-10-05 01:12:01 PDT
Sorry, no test, I don't know how news are supposed to work in this case.
But I run /mailnews/compose/test/unit/test_mailtoURL.js (as it references 'newshost') and it seems to still work.
Comment 9 Kent James (:rkent) 2012-10-09 10:38:56 PDT
Comment on attachment 668118 [details] [diff] [review]
patch

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

::: mailnews/compose/src/nsMsgSendLater.cpp
@@ +576,2 @@
>    if (m_newshost)
> +    fields->SetNewspostUrl(m_newshost);

This is the only change in this patch that may have an issue. Here's what I have discovered.

This gets executed when you send news messages sitting in the outbox that were "Sent Later". I could not figure out any effect that setting this has. However, there is a secondary bug that has been hidden by this.

If I post a simple Send Later news messages, I get two copies of the header X-Mozilla-News-Host in the outbox message, which is probably a bug:

From - Tue Oct 09 10:22:39 2012
...                                                                           
X-Mozilla-News-Host: news.mozilla.org
...
Newsgroups: mozilla.test
X-Mozilla-News-Host: 
...

As a result of this, the value that gets set by SetNewspostUrl has a comma after it:
"news.mozilla.org," which I can't believe is correct.

This is not your bug, but you are exposing it by removing the #if 0.  I could not find any negative consequences of this, but I also could not find any positive results of taking the risk to uncomment this code and its bug.

So r=me if you fix the code here as you have done, but leave it commented out. If you or jcranmer can convince me of the benefit of uncommenting this, or why the trailing comma is OK, then I could be persuaded otherwise. But if not, let's not risk a regression.
Comment 10 :aceman 2012-10-09 11:56:45 PDT
Created attachment 669672 [details] [diff] [review]
patch v2

I have no problem with leaving it commented out. I justed tried to poke anyone if he knows something about it and we can clean it up. I have no knowledge about news so will not try to persuade you :)
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-10-09 17:40:29 PDT
https://hg.mozilla.org/comm-central/rev/cd3a38d7b2b9

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