Closed Bug 760157 Opened 9 years ago Closed 8 years ago

Determining what is a feed message should use the new FeedMsg flag


(Thunderbird :: Folder and Message Lists, defect)

Not set


(Not tracked)

Thunderbird 29.0


(Reporter: alta88, Assigned: alta88)




(1 file, 2 obsolete files)

No description provided.
Attached patch flagFeedMsg.patch (obsolete) — Splinter Review
check for nsMsgMessageFlags::FeedMsg flag first to see if it's a feed message, not just the folder.

ps. thanks for all the recent reviews.
Assignee: nobody → alta88
Attachment #828221 - Flags: review?(mkmelin+mozilla)
Comment on attachment 828221 [details] [diff] [review]

Review of attachment 828221 [details] [diff] [review]:

How would a feed flagged message end up outside a feed folder? Is the flag copied if you copy the msg to elsewhere? 
Or what is the use case?

Looks ok to me, but someone else (standard8 perhaps?) should review this. And since the only use is in contentpolicy, it should probably have test coverage.
Attachment #828221 - Flags: review?(mkmelin+mozilla) → feedback+
The usual way, move or copy; the flag is retained like read or other flags in whatever folder/message organization the user chooses.  It seems wrong to treat a same message differently if moved to pop/local folders, especially since we know it's non mail based on the flag and not account.  (The flag doesn't seem to be kept for imap unfortunately; for all the feeds in the cloud talk it's often forgotten imap is the cloud too and it's easy enough to manage feeds that way if one wants.)

It seems a rather minor change to make Tb a bit smarter about content.  If a test is needed, perhaps a suggestion could be made as to what exactly it would test.  Currently a message is a feed if it 1) has the flag, 2) is in a feed account folder.  This is the only place that isn't true.
Flags: needinfo?(mbanner)
I'd add a message, set the feed flag for it and see that at least JavaScript works for that message.
I think that's yet another issue.  The situation is that remote content policy and js/plugins are determined separately for messagepane, and for feeds also affect the summary (mailbox://) or web page.  (Web page in messgepane means app type mail, which is different handling from http:// in a content tab.)

This particular function IsRSSArticle() is only (currently) used to check remote content (and put up that notification that seems to annoy people even in mail).  I'd hope the extra work of a test can be avoided since it's so binary and uncomplex in this case..

IsRSSArticle() should also, imo, be used to allow js/plugins for a mailbox:// iff it's a feed, but that's another bug/discussion.
The code looks fine. I think we do want a test for this, as if anyone refactors or reorganises the code, this is a highly unobvious case that could be overlooked.
Flags: needinfo?(mbanner)
So it's still not clear what a meaningful test would do?  Creating a message (in a non feed account folder) and adding the flag, would then just test if the msghdr flags passed a bitmask test for that flag.

Is that enough?
Since it is used to check remote content, maybe add something very similar to

... but when you get the msgDbHdr you set the feed flag
Attached patch flagFeedMsg.patch (obsolete) — Splinter Review
patch with test.
Attachment #828221 - Attachment is obsolete: true
Attachment #8350659 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8350659 [details] [diff] [review]

Review of attachment 8350659 [details] [diff] [review]:

Looks good! r=mkmelin

::: mail/test/mozmill/content-policy/test-general-content-policy.js
@@ +234,5 @@
>    if (mc.tabmail.tabContainer.childNodes.length != preCount)
>      throw new Error("The content tab didn't close");
>  }
> +function checkAllowFeedMsg(test) {

please add short function documentation to describe what it tests

@@ +243,5 @@
> +  // select the newly created message
> +  let msgHdr = select_click_row(gMsgNo);
> +
> +  if (msgDbHdr != msgHdr)
> +    throw new Error("Selected Message Header is not the same as generated header");

this could use assert_equals instead
Attachment #8350659 - Flags: review?(mkmelin+mozilla) → review+
Attachment #8350659 - Attachment is obsolete: true
Attachment #8355224 - Flags: review+
Keywords: checkin-needed
Closed: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
You need to log in before you can comment on or make changes to this bug.