Closed Bug 1116561 Opened 5 years ago Closed 5 years ago

filter after the fact should return an error if any filter failed

Categories

(MailNews Core :: Filters, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 37.0

People

(Reporter: rkent, Assigned: rkent)

Details

Attachments

(1 file, 1 obsolete file)

In bug 695671, a cleanup of nsMsgFilterAfterTheFact made error handling more explicit. But the assumption was that we would continue whenever possible after errors. In user-applied contexts (such as the After Sending filters of 11039), it would be good for the user to be informed of errors in filtering.

Modify nMsgFilterAfterTheFact so that if any error is noted, the operation callback returns a failure code.
Attached patch 11039RKJ.patch (obsolete) — Splinter Review
Assignee: nobody → kent
Status: NEW → ASSIGNED
Attachment #8542671 - Flags: review?(neil)
Comment on attachment 8542671 [details] [diff] [review]
11039RKJ.patch

>+#define BREAK_IF_FAILURE(_rv, _text) if (NS_FAILED(_rv)) { \
>+  NS_WARNING(_text); \
>+  mFinalResult = _rv; \
>+  break;}
(I don't like this formatting, can you put the } on its own line?)

>+#define BREAK_IF_FALSE(_assertTrue, _text) if (!(_assertTrue)) { \
>+    NS_WARNING(_text); \
>+    mFinalResult = NS_FAILED(rv) ? rv : NS_ERROR_FAILURE; \
>+    break;}
>+
>+#define CONTINUE_IF_FALSE(_assertTrue, _text) if (!(_assertTrue)) { \
>+    NS_WARNING(_text); \
>+    mFinalResult = NS_FAILED(rv) ? rv : NS_ERROR_FAILURE;\
>+    if (m_msgWindow && !ContinueExecutionPrompt()) \
>+      return OnEndExecution(); \
>+    continue;}
I don't like the hidden dependency on rv. I know there are a few cases that test rv and also something else, perhaps they can be split into _IF_FAILURE and _IF_FALSE cases?

>+
> };
Nit: don't like this blank line.

> /* void OnStopCopy (in nsresult aStatus); */
> NS_IMETHODIMP nsMsgFilterAfterTheFact::OnStopCopy(nsresult aStatus)
> {
>+  if (NS_FAILED(aStatus))
>+    mFinalResult = aStatus;
>   if (NS_FAILED(aStatus) && m_msgWindow && !ContinueExecutionPrompt())
>-    return OnEndExecution(aStatus);
>+    return OnEndExecution();
> 
>   return NS_SUCCEEDED(aStatus) ? ApplyFilter() : RunNextFilter();
> }
Can this be rewritten in the style of the other callbacks?
Attachment #8542671 - Attachment is obsolete: true
Attachment #8542671 - Flags: review?(neil)
Attachment #8542751 - Flags: review?(neil)
Comment on attachment 8542751 [details] [diff] [review]
Revise formatting of macros, no rv dependence

OK this seems fair enough.
Attachment #8542751 - Flags: review?(neil) → review+
Checked in http://hg.mozilla.org/comm-central/rev/5967bba9d130
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
OS: Windows 8.1 → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 37.0
You need to log in before you can comment on or make changes to this bug.