Closed Bug 522645 Opened 13 years ago Closed 10 years ago
Determining what is a feed message regression
1.08 KB, patch
|Details | Diff | Splinter Review|
1021 bytes, patch
|Details | Diff | Splinter Review|
2.38 KB, patch
|Details | Diff | Splinter Review|
the patch in Bug 438429 specifically handled the following cases: 1)a feed message can be in any folder, not just News 2)a feed message in a standalone window is recognized as such Bug 474701 removed the content-base check, which should be restored. in addition, there won't be a gFolderDisplay object in a standalone message window, i believe, so the selectedMessageIsFeed there isn't going to work.
> there won't be a gFolderDisplay object in a standalone message window There should be.
Can you describe exactly what the user-visible effects of this bug are?
xref bug 493221 which likely WFM now (by the same cause)
(In reply to comment #1) > > there won't be a gFolderDisplay object in a standalone message window > > There should be. yep, it's there. standalone message from News folder is recognized. (In reply to comment #2) > Can you describe exactly what the user-visible effects of this bug are? any feed message not in the News folder will not have feed specific menuitems displayed. this patch should revert to the spirit of the previous check. (In reply to comment #3) > xref bug 493221 which likely WFM now (by the same cause) imo it is far more important to enable feed messages in different folder organizations, than remove the check to address that bug, which is a quite nonstandard mail header issued by the publisher, and in fact not formatted to spec.
Comment on attachment 406713 [details] [diff] [review] restore content-base feed check. This code should probably be reviewed by myself, sid0, or bienvenu. You shouldn't use currentHeaderData here. Is there something on the nsIMsgDBHdr itself you can use?
Comment on attachment 406713 [details] [diff] [review] restore content-base feed check. No need for sr, since this is in mail/. Myk has graciously agreed to review, which is good, because I'm not turning around reviews as quickly as I'd like to be at the moment. Thanks, Myk!
Attachment #406713 - Flags: review- → review?(myk)
Mmmm, bugzilla-collision-riffic. Will aim review at bienvenu.
Assignee: nobody → alta77
Attachment #406713 - Flags: review?(myk) → review?(bienvenu)
Comment on attachment 406713 [details] [diff] [review] restore content-base feed check. We don't put the content base in the msg hdr - currentHeaderData could be empty if there's no message pane displayed.
Right, this sounds familiar. I think I tried to avoid removing the check in the code but there was no easy way to reliably have the data. The best-case solution for this is to have the MessageDisplayWidget register for header notifications from the message reader. If it sees that header is set, it can set an internal flag like "bodyContentIndicatesFeed" which the FolderDisplayWidget can check. It could also trigger a toolbar update or whatever is required if it can't rely on one being produced elsewhere (although I suspect it can). We definitely want to avoid having FolderDisplayWidget access a global provided by the message reader without active/inactive scoping and without some awareness of the availability of the data being event-driven.
Comment on attachment 406713 [details] [diff] [review] restore content-base feed check. minusing based on asuth's comments.
Should we consider putting the Content-Base into the message header?
We could, since only rss messages will have content-base. It wouldn't help with already downloaded rss messages, however, w/o a reparse.
hmm, the new internals look quite different from what i recall. since the purpose here is to only do the check/show menuitems if 1 message is selected (not meaningful for possible mixed types in multiselect), it seems reasonable to require that message pane or message in standalone window is visible. in which case header data should be very easily accessible and currentHeaderData should work. otherwise the menuitems can be disabled. any reason not to just go back to the prior method?
See Andrew's comment #9 >We definitely want to avoid having FolderDisplayWidget access a global provided >by the message reader without active/inactive scoping and without some >awareness of the availability of the data being event-driven.
right, i took that to mean using gFolderDisplay methods in folderDisplay.js, not the already available header data in mailWindowOverlay.js. perhaps incorrectly..
ah, by prior method, you mean going back to code in mailWindowOverlay.js...I don't know this code that well, asuth should probably chime in.
(In reply to comment #15) > right, i took that to mean using gFolderDisplay methods in folderDisplay.js, > not the already available header data in mailWindowOverlay.js. perhaps > incorrectly.. In an ideal world we could identify messages that are RSS messages without streaming them. It would be most preferable to move towards that world, which would mean setting the message header and just using that. (Given that RSS is a 'timely' medium, even if users do not reindex, most would probably not even have a problem since all the new messages would still get the header.) Having said that, mailWindowOverlay.js is still an untamed wilderness from my perspective; I'm fine with doing the currentHeaderData check there. The caveat is that when we actually improve the message reader situation, it may get broken again unless RSS becomes a priority and starts getting a lot of love and unit tests. The 'more correct' solution is less likely to get steamrolled.
yeah, I'm happy to make the local message parser set a content-base property and let alta88 query that property. I'll put up a patch in a minute for him to play with.
david, i don't know that you need to do anything, it would just go back to what worked before. content-base was just available, filled in when a message was selected.. the thing that would be most correct to do, and take no time, would be to not depend on content-base, since per strict rfc it is freeform and an email client *could* publish it, though it's very much not customary. it would be simple to add something like x-moz-feed and fill it with something useful like rss/atom version etc. then we could check for x-moz-feed instead of content-base.
alta88, if you run with this patch, newly downloaded rss messages should have a string property set to the content-base header value, so nsIMsgDBHdr.getStringProperty("content-base") would tell if you if the message has a content base.
ah, didn't see your comment til after. The same patch could be tweaked to add x-moz-feed instead...but you'd need to set that header in the feed code, as you know, I'm sure.
What would be even nicer is to make nsILocalMailFolder::addMessage return the msg hdr of the header added, and you could set the property yourself. I think I'm going to do that. That would also make some of our test cases easier to write.
Assuming we use Content-Base this way, what are the security implications (if any) of a black-hat/phisher sending an email with a Content-Base header pointing to site of their choosing. Is there any chance we'd load that site rather than displaying the message itself?
(In reply to comment #23) > Assuming we use Content-Base this way, what are the security implications (if > any) of a black-hat/phisher sending an email with a Content-Base header > pointing to site of their choosing. Is there any chance we'd load that site > rather than displaying the message itself? yes, if the default were to show web page. the issue exists in Tb2 as well. and it would be true if this functionality depended on any header; x-moz-feed could be spoofed in an email too. feedparser or whatever stores a msg to the db would have to set a feed flag, which could be trusted, or similar arch..
filed bug 647699, as per c#22 it's the best way to go. we really need a trusted way of identifying feed messages (not based on folder).
Depends on: 647699
now that 647699 is fixed thanks to magnus, i'd like to complete this, such that a message that is a feed is treated as such regardless of folder server (including re content policy). it seems there is a free slot at x40 for an isFeed flag here: http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsMsgMessageFlags.idl#43 david, is it ok to take this bit? initially i thought there could be a separate feed uint property with other flags but couldn't think of any other flags.
(In reply to alta88 from comment #27) > david, is it ok to take this bit? initially i thought there could be a > separate feed uint property with other flags but couldn't think of any other > flags. yes, I'm OK with that; it seems like a relatively important attribute worthy of one of our precious remaining bits :-) Or you could also simply use a property on the nsIMsgDBHdr, that defaults to false and is only set for feeds. It's marginally slower, and adds a bit of bloat to the .msf files for feeds
yeah, if there were any reason to store anything more informative, a property would be the way to go, but there isn't. so this is probably a 'bit' better. part 1 to merely store the flag.
Assignee: alta77 → alta88
Attachment #628166 - Flags: review?(dbienvenu)
Comment on attachment 628166 [details] [diff] [review] patch looks OK. I forget what 0x40 used to be used for. It looks like it's been unused for a very long time, but there's a small chance it might exist out there in some local mail folders in x-mozilla-status headers.
Attachment #628166 - Flags: review?(dbienvenu) → review+
if that's indeed the case, subsequent code can also check to see if there's a content-base header. making use of the new flag will be a separate bug.
Landed in comm-central as https://hg.mozilla.org/comm-central/rev/81cb0eec3150
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
You need to log in before you can comment on or make changes to this bug.