556786 - Filter junk status "Not Junk" won't save

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Kent James (:rkent)]

STR:

Setup a new filter with filter action "Junk Status" is "Not Junk". Exit saving the filter, and restart. The filter will have "Junk" instead of "Not Junk" as the status.

Came across this trying to fix bug 540385, regression from a string-related change.

Comment 1

[Kent James (:rkent)]

I'm going to remove this from my patch for bug 540385, and land it later. Here is the chunk:

diff --git a/mailnews/base/search/src/nsMsgFilterList.cpp b/mailnews/base/search/src/nsMsgFilterList.cpp
--- a/mailnews/base/search/src/nsMsgFilterList.cpp
+++ b/mailnews/base/search/src/nsMsgFilterList.cpp
@@ -687,17 +687,17 @@ nsresult nsMsgFilterList::LoadTextFilter
             currentFilterAction->SetType(nsMsgFilterAction::AddTag);
             currentFilterAction->SetStrValue(keyword);
           }
         }
         else if (type == nsMsgFilterAction::JunkScore)
         {
           nsresult res;
           PRInt32 junkScore = value.ToInteger(&res, 10);
-          if (NS_FAILED(res))
+          if (NS_SUCCEEDED(res))
             currentFilterAction->SetJunkScore(junkScore);
         }
         else if (type == nsMsgFilterAction::Forward ||
                  type == nsMsgFilterAction::Reply ||
                  type == nsMsgFilterAction::AddTag ||
                  type == nsMsgFilterAction::Custom)
         {
           currentFilterAction->SetStrValue(value);

Comment 2

[Kent James (:rkent)]

Brief history of this bug: the original error was actually introduced in 2008 in bug 395701 - but it only affected cases where MOZILLA_INTERNAL_API was not set, which meant no normal uses.

Recently, the 2010-03-03 checkin of bug 377319 removed the MOZILLA_INTERNAL_API check, and exposed the problem introduced by the previous bug. I think it is worth noting that there are lots of places in the code where previous dual internal/external string code existed, but the external code was virtually untested. So this may not be the only regression introduced by recent removals of MOZILLA_INTERNAL_API.

Comment 3

[Kent James (:rkent)]

Attachment: NS_FAILED -> NS_SUCCEEDED

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 436994 [details] [diff] [review]
NS_FAILED -> NS_SUCCEEDED

Stealing review as this is simple and I'd like to see it fixed.

I actually found the STR were to set the junk status to "Junk" and then it would be "Not Junk" after a restart. Maybe we'd started from slightly different setups.

Still, with the patch it works nicely. r/sr=Standard8.

Comment 5

[Kent James (:rkent)]

Comment on attachment 436994 [details] [diff] [review]
NS_FAILED -> NS_SUCCEEDED

Chced in http://hg.mozilla.org/comm-central/rev/ea7e7431ebfd