Closed
Bug 133605
Opened 22 years ago
Closed 12 years ago
nsIMsgCompFields::[Set|Get]NewsHost is identical to nsIMsgCompFields::[Set|Get]NewspostUrl
Categories
(MailNews Core :: Composition, defect)
MailNews Core
Composition
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...
Reporter | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.2alpha
QA Contact: esther → stephend
Updated•20 years ago
|
Product: MailNews → Core
Updated•16 years ago
|
Assignee: ducarroz → nobody
Status: ASSIGNED → NEW
QA Contact: stephend → composition
Updated•16 years ago
|
Product: Core → MailNews Core
Comment 2•13 years ago
|
||
(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 → ---
Comment 3•13 years ago
|
||
We appear to universally use NewspostUrl instead of NewsHost, so I would consider that a good reason :-)
Updated•13 years ago
|
Whiteboard: [good first bug]
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•12 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)
Attachment #668118 -
Flags: review?(mkmelin+mozilla)
Attachment #668118 -
Flags: review?(kent)
Attachment #668118 -
Flags: review?(Pidgeot18)
Attachment #668118 -
Flags: feedback?(Pidgeot18)
Comment 7•12 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)
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•12 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•12 years ago
|
||
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]
Comment 11•12 years ago
|
||
https://hg.mozilla.org/comm-central/rev/cd3a38d7b2b9
Status: ASSIGNED → RESOLVED
Closed: 12 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.
Description
•