Closed Bug 481676 Opened 11 years ago Closed 11 years ago

Move bayes listeners to nsMsgDBFolder

Categories

(MailNews Core :: Filters, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: rkent, Assigned: rkent)

References

Details

Attachments

(2 files, 2 obsolete files)

There is a lot of common code that is used by the bayes junk and trait listeners, that currently resides in the local and imap folder code. I want to extend bayes processing to other code that uses nsMsgDBFolder, such as rss and news. Rather than duplicate that code, let's pull out the common code and move it into nsMsgDBFolder. Then the addition of bayes processing to rss and news becomes rather trivial, at least from the backend.

So this is a big ugly patch because a lot of code is being moved, yet very little new is really being introduced. To hopefully make the review easier, I've split off this particular refactoring from bug 471833. Then we can focus there more precisely on the question of how to enable bayes processing using the inherited folder properties of bug 478360.
This patch looks big and harry, but most of it is just moving code up one level in the inheritance chain. I struggle to get all of the XPCOM macros correct, but hopefully I got it this time.

One new thing is that instead of counting messages to know when we are finished with a batch of junk analysis, I let the bayes filter inform the listener by sending a null URI as a signal of end of batch. I think that will be more reliable than a count. There was some other difficulty with the count that I've forgotten.

With this change, enabling backend support for junk and trait processing in news and rss (which would not support moves anyway) is just a matter of adding one-line calls at the appropriate places to CallFilterPlugins.
Assignee: nobody → kent
Status: NEW → ASSIGNED
Attachment #365737 - Flags: superreview?(bienvenu)
Attachment #365737 - Flags: review?(neil)
Whiteboard: [needs r neil, sr bienvenu]
Attachment #365737 - Flags: review?(neil) → review+
Comment on attachment 365737 [details] [diff] [review]
Consolidate code into nsMsgDBFolder

>+NS_IMETHODIMP
>+nsMsgDBFolder::OnMessageClassified(const char *aMsgURI,
>+  nsMsgJunkStatus aClassification,
>+  PRUint32 aJunkPercent)
As I recall the preferred indentation is something like this:
NS_IMETHODIMP
nsMsgDBFolder::OnMessageClassified(const char *aMsgURI,
                                   nsMsgJunkStatus aClassification,
                                   PRUint32 aJunkPercent)

>+  nsresult rv;
> 
>   nsCOMPtr<nsIMsgIncomingServer> server;
>-  nsresult rv = GetServer(getter_AddRefs(server));
...
>+  rv = GetServer(getter_AddRefs(server));
Why this change?

>+  nsCString uriStr;
>+  rv = spamSettings->GetSpamFolderURI(getter_Copies(uriStr));
>+  NS_ENSURE_SUCCESS(rv,rv);
>+  mSpamFolderURI = uriStr;
I wonder why mSpamFolderURI exists. It's never used outside this method.
(i.e. you could use uriStr throughout, although I'd rename it spamFolderURI)
(In reply to comment #2)
> >+  nsresult rv;
> > 
> >   nsCOMPtr<nsIMsgIncomingServer> server;
> >-  nsresult rv = GetServer(getter_AddRefs(server));
> ...
> >+  rv = GetServer(getter_AddRefs(server));
> Why this change?

Probably just leftover after a bunch of rearrangements. I'll put it back to the original.

> I wonder why mSpamFolderURI exists. It's never used outside this method.

Good point, I'll change that (though that is just copied code).

Revised patch coming.
Attached patch Fixed Neil's issues (obsolete) — Splinter Review
Attachment #365737 - Attachment is obsolete: true
Attachment #366347 - Flags: superreview?(bienvenu)
Attachment #366347 - Flags: review+
Attachment #365737 - Flags: superreview?(bienvenu)
Whiteboard: [needs r neil, sr bienvenu] → [needs sr bienvenu]
Attachment #366347 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 366347 [details] [diff] [review]
Fixed Neil's issues

a few nits:

I like the args to line up, as long as it doesn't make the lines too long.

+  void onMessageClassified(in string aMsgURI,
       in nsMsgJunkStatus aClassification,
       in PRUint32 aJunkPercent);


I know you're just moving code, but I don't think you need to init the comptr to nsnull here:

+          nsCOMPtr<nsIMsgDBHdr> mailHdr = nsnull;

should check Out of memory failure case here:

+          if (!m_junkMessagesToMarkAsRead)
+            m_junkMessagesToMarkAsRead = do_CreateInstance(NS_ARRAY_CONTRACTID);
+          m_junkMessagesToMarkAsRead->AppendElement(msgHdr, PR_FALSE);
Carrying over r/sr, patch to checkin. Thanks for the reviews, Neil and David!
Attachment #366347 - Attachment is obsolete: true
Attachment #366855 - Flags: superreview+
Attachment #366855 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs sr bienvenu] → [needs checkin]
Target Milestone: --- → Thunderbird 3.0b3
Checked in: http://hg.mozilla.org/comm-central/rev/996cdbab71a9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs checkin]
Linux tbox is showing a breakage due to the main landed patch. I believe this will fix that test.
Attachment #366910 - Flags: review?(bugzilla)
Comment on attachment 366910 [details] [diff] [review]
Fix broken test_366491.js on Linux by adding a end of batch test

Thanks for doing that, r=me.

Checked in: http://hg.mozilla.org/comm-central/rev/2dc313097d22
Attachment #366910 - Flags: review?(bugzilla) → review+
You need to log in before you can comment on or make changes to this bug.