Closed Bug 471559 Opened 16 years ago Closed 15 years ago

Combine code for pre-Bayesian junk and trait decisions (whitelisting, etc.)

Categories

(MailNews Core :: Filters, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: rkent, Assigned: rkent)

References

Details

Attachments

(1 file, 2 obsolete files)

Currently, there is code in C++ and js that do essentially the same thing, running though a list of issues that determine if junk mail should be classified independently of the Bayesian filter (through user or domain whitelisting, for example), and then determine if the Bayesian filter is to run at all. I need to do some updates to that code, including bug 452879 as well as allowing Bayesian processing to run for traits even if junk status is already known. Rather than contribute to the existing duplication, I would like to combine this into a single component.
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0b3
Attachment #368933 - Flags: superreview?(bugzilla)
Attachment #368933 - Flags: review?(bugzilla)
Attached patch change guid of changed interface (obsolete) — Splinter Review
In looking at my diff on bugzilla, I noticed that I forgot to change the guid of the changed idl.
Attachment #368933 - Attachment is obsolete: true
Attachment #368935 - Flags: superreview?(bugzilla)
Attachment #368935 - Flags: review?(bugzilla)
Attachment #368933 - Flags: superreview?(bugzilla)
Attachment #368933 - Flags: review?(bugzilla)
Whiteboard: [needs r/sr standard8]
Comment on attachment 368935 [details] [diff] [review]
change guid of changed interface

I like this, nice tidy up. Generally, looks good, just a couple of things that need fixing up.

>+    // check whitelisting
>+    try
>     {
...
>+      if (aSpamSettings.checkWhiteList(aMsgHdr))
>       {
...
>+        // message is ham from whitelist
>+        var db = aMsgHdr.folder.msgDatabase;
>+        db.setStringProperty(aMsgHdr.messageKey, "junkscore",
>+                             Components.interfaces.nsIJunkMailPlugin.IS_HAM_SCORE);
>+        db.setStringProperty(aMsgHdr.messageKey, "junkscoreorigin", "whitelist");
>+        this.mGoodMsgHdrs.appendElement(aMsgHdr, false);
>+        return;
>       }
>-    }
>+    } catch (e) {}

AFAICT none of these functions will throw under normal conditions. Therefore the try/catch shouldn't be used, as it will just hide actual errors.

>+    nsCOMPtr <nsIRDFService> rdfService =
>+      do_GetService("@mozilla.org/rdf/rdf-service;1",&rv);
>+    NS_ENSURE_SUCCESS(rv, rv);

Please use nsIAbManager and its getDirectory function to get the directory from a URI (I've been progressively "hiding" rdf behind the nsIAbManager function).

nit: no space before <

>+  nsCString author;
>+  aMsgHdr->GetAuthor(getter_Copies(author));
>+  nsresult rv;
>+  nsCOMPtr<nsIMsgHeaderParser> headerParser =
>+    do_GetService(NS_MAILNEWS_MIME_HEADER_PARSER_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  nsCAutoString authorEmailAddress;
>+  rv = headerParser->ExtractHeaderAddressMailboxes(author, authorEmailAddress);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (authorEmailAddress.IsEmpty())
>+    return NS_OK;

nit: insert a blank line after each NS_ENSURE_SUCCESS please.

>+  if (mWhiteListDirArray.Count() != 0)

nit: can just be if (mWhiteListDirArray.Count())

>+    nsCOMPtr<nsIAbCard> cardForAddress;
>+    for (PRInt32 index = 0;
>+         index < mWhiteListDirArray.Count() && !cardForAddress;
>+         index++)

This doesn't need the check for !cardForAddress as you're returning out of the for loop if you find the card.
Attachment #368935 - Flags: superreview?(bugzilla)
Attachment #368935 - Flags: review?(bugzilla)
Attachment #368935 - Flags: review-
(In reply to comment #3)
> 
> AFAICT none of these functions will throw under normal conditions. Therefore
> the try/catch shouldn't be used, as it will just hide actual errors.
>
I've been slowly changing my philosophy toward error handling recently, focusing on responding safely rather than causing exceptions that kill processing. But this is probably not the best way to propose that philosophy, so I've removed these.

> >+  if (mWhiteListDirArray.Count() != 0)
> 
> nit: can just be if (mWhiteListDirArray.Count())

I'm also changing a related call to this earlier.
> 
> >+    nsCOMPtr<nsIAbCard> cardForAddress;
> >+    for (PRInt32 index = 0;
> >+         index < mWhiteListDirArray.Count() && !cardForAddress;
> >+         index++)
> 
> This doesn't need the check for !cardForAddress as you're returning out of the
> for loop if you find the card.

I don't think that's true, the for loop is only a single line. I added brackets to clarify. The style guidelines to skip the brackets are not clear when the for statement itself is multiline.
Status: NEW → ASSIGNED
Attachment #368935 - Attachment is obsolete: true
Attachment #369361 - Flags: superreview?(bugzilla)
Attachment #369361 - Flags: review?(bugzilla)
(In reply to comment #4)
> (In reply to comment #3)
> > 
> > AFAICT none of these functions will throw under normal conditions. Therefore
> > the try/catch shouldn't be used, as it will just hide actual errors.
> >
> I've been slowly changing my philosophy toward error handling recently,
> focusing on responding safely rather than causing exceptions that kill
> processing. But this is probably not the best way to propose that philosophy,
> so I've removed these.

Maybe I was a bit hard in my comment - I'm not too much against this philosophy, but just dumping exceptions to /dev/null is not acceptable - we've had cases where we would have spotted something much earlier had it not been for a try with an empty catch. Though I do think in this case something would have gone wrong earlier for us to get an error there.

> > >+    nsCOMPtr<nsIAbCard> cardForAddress;
> > >+    for (PRInt32 index = 0;
> > >+         index < mWhiteListDirArray.Count() && !cardForAddress;
> > >+         index++)
> > 
> > This doesn't need the check for !cardForAddress as you're returning out of the
> > for loop if you find the card.
> 
> I don't think that's true, the for loop is only a single line. I added brackets
> to clarify. The style guidelines to skip the brackets are not clear when the
> for statement itself is multiline.

Yeah, I read it wrong the first time round. I think in this case it is slightly clearer to have the brackets.
Attachment #369361 - Flags: superreview?(bugzilla)
Attachment #369361 - Flags: superreview+
Attachment #369361 - Flags: review?(bugzilla)
Attachment #369361 - Flags: review+
Thanks for the patch, I've checked this in for you: http://hg.mozilla.org/comm-central/rev/1d0a297584ff
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs r/sr standard8]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: