nsIMsgCompFields::[Set|Get]NewsHost is identical to nsIMsgCompFields::[Set|Get]NewspostUrl

RESOLVED FIXED in Thunderbird 19.0

Status

MailNews Core
Composition
--
minor
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: Jean-Francois Ducarroz, Assigned: aceman)

Tracking

Trunk
Thunderbird 19.0
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

4.81 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
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...
(Reporter)

Updated

15 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.2alpha
QA Contact: esther → stephend

Comment 1

14 years ago
not a mac classic bug
OS: Mac System 9.x → All
Product: MailNews → Core
Assignee: ducarroz → nobody
Status: ASSIGNED → NEW
QA Contact: stephend → composition
Product: Core → MailNews Core
(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
Severity: normal → minor
Target Milestone: mozilla1.2alpha → ---
We appear to universally use NewspostUrl instead of NewsHost, so I would consider that a good reason :-)
Whiteboard: [good first bug]
(Assignee)

Comment 4

6 years ago
This still seems relevant, should I try this?
(Assignee)

Comment 5

5 years ago
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?
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #668118 - Flags: superreview?(mkmelin+mozilla)
Attachment #668118 - Flags: review?(Pidgeot18)

Comment 6

5 years ago
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.
Attachment #668118 - Flags: superreview?(mkmelin+mozilla)
(Assignee)

Updated

5 years ago
Attachment #668118 - Flags: review?(mkmelin+mozilla)
Attachment #668118 - Flags: review?(kent)
Attachment #668118 - Flags: review?(Pidgeot18)
Attachment #668118 - Flags: feedback?(Pidgeot18)

Comment 7

5 years ago
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?
Attachment #668118 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 8

5 years ago
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

5 years ago
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.
Attachment #668118 - Flags: review?(kent) → review+
(Assignee)

Comment 10

5 years ago
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 :)
Attachment #668118 - Attachment is obsolete: true
Attachment #668118 - Flags: feedback?(Pidgeot18)
Attachment #669672 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [good first bug]
https://hg.mozilla.org/comm-central/rev/cd3a38d7b2b9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.0
You need to log in before you can comment on or make changes to this bug.