Closed Bug 133605 Opened 19 years ago Closed 8 years ago

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

Categories

(MailNews Core :: Composition, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 19.0

People

(Reporter: bugzilla, Assigned: aceman)

Details

Attachments

(1 file, 1 obsolete file)

4.81 KB, patch
aceman
: review+
Details | Diff | Splinter Review
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...
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.2alpha
QA Contact: esther → stephend
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]
This still seems relevant, should I try this?
Attached patch patch (obsolete) — Splinter Review
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 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)
Attachment #668118 - Flags: review?(mkmelin+mozilla)
Attachment #668118 - Flags: review?(kent)
Attachment #668118 - Flags: review?(Pidgeot18)
Attachment #668118 - Flags: feedback?(Pidgeot18)
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)
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 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+
Attached patch patch v2Splinter Review
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+
Keywords: checkin-needed
Whiteboard: [good first bug]
https://hg.mozilla.org/comm-central/rev/cd3a38d7b2b9
Status: ASSIGNED → RESOLVED
Closed: 8 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.