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

RESOLVED FIXED in Thunderbird 29.0

Status

Thunderbird
Folder and Message Lists
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: alta88, Assigned: alta88)

Tracking

unspecified
Thunderbird 29.0
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 828221 [details] [diff] [review]
flagFeedMsg.patch


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 2

4 years ago
Comment on attachment 828221 [details] [diff] [review]
flagFeedMsg.patch

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+
(Assignee)

Comment 3

4 years ago
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)

Comment 4

4 years ago
I'd add a message, set the feed flag for it and see that at least JavaScript works for that message.
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/content-policy/test-general-content-policy.js
(Assignee)

Comment 5

4 years ago
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)
(Assignee)

Comment 7

4 years ago
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?

Comment 8

4 years ago
Since it is used to check remote content, maybe add something very similar to 
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/content-policy/test-general-content-policy.js#200

... but when you get the msgDbHdr you set the feed flag
(Assignee)

Comment 9

4 years ago
Created attachment 8350659 [details] [diff] [review]
flagFeedMsg.patch


patch with test.
Attachment #828221 - Attachment is obsolete: true
Attachment #8350659 - Flags: review?(mkmelin+mozilla)

Comment 10

4 years ago
Comment on attachment 8350659 [details] [diff] [review]
flagFeedMsg.patch

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+
(Assignee)

Comment 11

4 years ago
Created attachment 8355224 [details] [diff] [review]
flagFeedMsg.patch
Attachment #8350659 - Attachment is obsolete: true
Attachment #8355224 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/e85ea148b04d
Status: NEW → RESOLVED
Last Resolved: 4 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.