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)
MailNews Core
Filters
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b3
People
(Reporter: rkent, Assigned: rkent)
References
Details
Attachments
(1 file, 2 obsolete files)
20.68 KB,
patch
|
standard8
:
review+
standard8
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•15 years ago
|
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0b3
Assignee | ||
Comment 1•15 years ago
|
||
Attachment #368933 -
Flags: superreview?(bugzilla)
Attachment #368933 -
Flags: review?(bugzilla)
Assignee | ||
Comment 2•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs r/sr standard8]
Comment 3•15 years ago
|
||
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-
Assignee | ||
Comment 4•15 years ago
|
||
(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
Assignee | ||
Comment 5•15 years ago
|
||
Attachment #368935 -
Attachment is obsolete: true
Attachment #369361 -
Flags: superreview?(bugzilla)
Attachment #369361 -
Flags: review?(bugzilla)
Comment 6•15 years ago
|
||
(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.
Updated•15 years ago
|
Attachment #369361 -
Flags: superreview?(bugzilla)
Attachment #369361 -
Flags: superreview+
Attachment #369361 -
Flags: review?(bugzilla)
Attachment #369361 -
Flags: review+
Comment 7•15 years ago
|
||
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.
Description
•