555051 - Support "needs body" for custom filter actions

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement)

Description

[Kent James (:rkent)]

Custom searches can request "needs body" which results in the delay of the filter until after messages are downloaded. Custom filter actions need the same capability in nsIMsgFilterCustomAction

Comment 1

[Kent James (:rkent)]

I have a patch for this, but I'm having second thoughts about whether I really need this.

Comment 2

[Kent James (:rkent)]

I implemented today a Body Regular Expression search in FiltaQuilla using the equivalent needsBody attribute in custom search terms, and I am very pleased with that (see also bug 19442 with 104 votes!). Without needsBody, it hung on imap. Not sure why, maybe trying to start an IMAP url to download the body while we are in the middle of header processing doesn't work.

But the upshot is that I DO want this feature for custom actions as well, as many actions may also want the body available. So I will go ahead and post the patch and review request.

Comment 3

[Kent James (:rkent)]

Attachment: needsBody attribute and unit test

Comment 4

[Kent James (:rkent)]

Bienvenu: this turns out to be an important bug for me for TB 3.1, as without it I can't really do any filter actions that need the body (such as save attachment) in IMAP. It might also affect Calendar uses that need to process message attachments.

Comment 5

[David :Bienvenu]

Comment on attachment 435423 [details] [diff] [review]
needsBody attribute and unit test

+          nsCOMPtr<nsIMsgRuleAction> action;
+          rv = actionList->QueryElementAt(actionIndex,
+                                          NS_GET_IID(nsIMsgRuleAction),
+                                          getter_AddRefs(action));

can't this be

nsCOMPtr<<nsIMsgRuleAction> action(do_QueryElementAt(actionList, actionIndex, &rv));

I know you're just cloning tests, but I don't think this part is true or needed:

+  // these hacks are required because we've created the inbox before
+  // running initial folder discovery, and adding the folder bails
+  // out before we set it as verified online, so we bail out, and
+  // then remove the INBOX folder since it's not verified.
+  msgImapFolder.hierarchyDelimiter = '/';
+  msgImapFolder.verifiedAsOnlineFolder = true;
+

Other than that, it looks fine, and I'll give the patch and unit test a try.

Comment 6

[David :Bienvenu]

Comment on attachment 435423 [details] [diff] [review]
needsBody attribute and unit test

Oh, the fact that you need this change means a caller is not checking the rv correctly. But it looks to me like you're using a comptr in the calling code so you shouldn't need this change?

Comment 7

[David :Bienvenu]

Comment on attachment 435423 [details] [diff] [review]
needsBody attribute and unit test

r/sr=me, modulo previous comments.

Comment 8

[Kent James (:rkent)]

(In reply to comment #6)
> (From update of attachment 435423 [details] [diff] [review])
> Oh, the fact that you need this change means a caller is not checking the rv
> correctly. But it looks to me like you're using a comptr in the calling code so
> you shouldn't need this change?

If you mean:
-      NS_ERROR("Need to set CustomId");
+      *aCustomAction = nsnull;

I needed that change so that I could simply do the following:

+          nsCOMPtr<nsIMsgFilterCustomAction> customAction;
+          action->GetCustomAction(getter_AddRefs(customAction));
+          if (!customAction)
+            continue;

that checks for the customAction. The NS_ERROR caused some issue, perhaps it
caused the unit test to fail, can't recall. I don't see that I need it, if you
want I could change it to an NS_WARNING though. I still want to set the null
return though, as that just makes it easier to check for the customId in a
single operation. This is a critical point in the message processing, and I am
not going to do anything other than continue regardless of the error, so I
really don't need to consider the rv itself.

Comment 9

[David :Bienvenu]

it's bad xpcom style, unfortunately. If there's an error, you shouldn't make any assumptions about the out params. NS_WARNING would be good.

Comment 10

[Kent James (:rkent)]

Attachment: Fixed nits

Comment 11

[Kent James (:rkent)]

Comment on attachment 436606 [details] [diff] [review]
Fixed nits

Checked in http://hg.mozilla.org/comm-central/rev/770edf794261