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...
Product: MailNews Core
Classification: Components
Component: Composition (show other bugs)
: Trunk
: All All
-- minor (vote)
: Thunderbird 19.0
Assigned To: :aceman
Depends on:
  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:
QA Whiteboard:
Iteration: ---
Points: ---

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 User image 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 User image Simon Paquet [:sipaq] 2003-08-14 17:36:53 PDT
not a mac classic bug
Comment 2 User image 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?^[^\0]*%24&hitlimit=&tree=comm-central
Comment 3 User image 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 User image :aceman 2011-11-06 12:49:43 PST
This still seems relevant, should I try this?
Comment 5 User image :aceman 2012-10-04 12:28:47 PDT
Created attachment 668118 [details] [diff] [review]

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

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

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 User image Magnus Melin 2012-10-05 00:55:48 PDT
Comment on attachment 668118 [details] [diff] [review]

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 User image :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 User image Kent James (:rkent) 2012-10-09 10:38:56 PDT
Comment on attachment 668118 [details] [diff] [review]

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
Newsgroups: mozilla.test

As a result of this, the value that gets set by SetNewspostUrl has a comma after it:
"," 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 User image :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 User image Ryan VanderMeulen [:RyanVM] 2012-10-09 17:40:29 PDT

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