Consolidate UpdateSummaryTotals into nsMsgDBFolder

RESOLVED FIXED in mozilla1.9.1a1

Status

--
trivial
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: rkent, Assigned: rkent)

Tracking

Trunk
mozilla1.9.1a1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

10 years ago
A comment in nsImapMailFolder::UpdateSummaryTotals says:

// could we move this into nsMsgDBFolder, or do we need to deal
// with the pending imap counts?

In fact the code is duplicated in several places, and can be effectively moved. Since I've been reviewing handling of counts, I want to do this to make changes easier.
(Assignee)

Comment 1

10 years ago
Created attachment 326853 [details] [diff] [review]
UpdateSummaryTotals moved to parent

Although there were subtle differences between the versions, as far as I can tell they were not significant. The pending messages are set to zero. The check for mIsServer in:

if (!mNotifyCountChanges || mIsServer)

only occurred in the IMAP version, but I believe is also valid in News and Local, right?
Attachment #326853 - Flags: superreview?(bienvenu)
Attachment #326853 - Flags: review?(bienvenu)

Comment 2

10 years ago
Comment on attachment 326853 [details] [diff] [review]
UpdateSummaryTotals moved to parent

thx, Kent, yes, this looks right.
Attachment #326853 - Flags: superreview?(bienvenu)
Attachment #326853 - Flags: superreview+
Attachment #326853 - Flags: review?(bienvenu)
Attachment #326853 - Flags: review+

Updated

10 years ago
Keywords: checkin-needed
Checking in mailnews/base/util/nsMsgDBFolder.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgDBFolder.cpp,v  <--  nsMsgDBFolder.cpp
new revision: 1.350; previous revision: 1.349
done
Checking in mailnews/imap/src/nsImapMailFolder.cpp;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.cpp,v  <--  nsImapMailFolder.cpp
new revision: 1.822; previous revision: 1.821
done
Checking in mailnews/imap/src/nsImapMailFolder.h;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.h,v  <--  nsImapMailFolder.h
new revision: 1.254; previous revision: 1.253
done
Checking in mailnews/local/src/nsLocalMailFolder.cpp;
/cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.cpp,v  <--  nsLocalMailFolder.cpp
new revision: 1.593; previous revision: 1.592
done
Checking in mailnews/local/src/nsLocalMailFolder.h;
/cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.h,v  <--  nsLocalMailFolder.h
new revision: 1.166; previous revision: 1.165
done
Checking in mailnews/news/src/nsNewsFolder.cpp;
/cvsroot/mozilla/mailnews/news/src/nsNewsFolder.cpp,v  <--  nsNewsFolder.cpp
new revision: 1.300; previous revision: 1.299
done
Checking in mailnews/news/src/nsNewsFolder.h;
/cvsroot/mozilla/mailnews/news/src/nsNewsFolder.h,v  <--  nsNewsFolder.h
new revision: 1.103; previous revision: 1.102
done
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.1a1
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
There are plenty of errors of this sort on the console now:
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file D:/mozilla-build/msys/co/moz-expt/mailnews/base/util/nsMsgDBFolder.cpp, line 3286 (3283 on trunk)

Earlier the rv of ReadDBFolderInfo wasn't being tested, now it is.
(Assignee)

Comment 5

10 years ago
I'm seeing this now on startup - do you see it any other time?

It's being called from GetSubfolders/UpdateSummaryTotals, which then ignores the error return. So it is a false assertion. The problem would go away if we had force=TRUE - but Bienvenu added that in 1999 for some reason.

Any idea why the force=false is needed here (http://mxr.mozilla.org/mozilla/source/mailnews/local/src/nsLocalMailFolder.cpp#431):

   UpdateSummaryTotals(PR_FALSE);

We could also just ignore the error, but I think it would be better to understand what is going on.

Comment 6

10 years ago
Calling it with force=true forces us to open the db - that change would cause us to open the db of every local folder during folder discovery, which would pretty much make the cache useless.
(In reply to comment #5)
> We could also just ignore the error, but I think it would be better to
> understand what is going on.
> 

I stepped through the code a bit, and what's happening is that in normal operation, ReadDBFolderInfo() is entered:
http://mxr.mozilla.org/mozilla/source/mailnews/base/util/nsMsgDBFolder.cpp#496

Now none of the two if blocks are actually entered, so NS_ERROR_FAILURE is returned.

Absolutely nowhere else is the rv from ReadDBFolderInfo tested (verified using mxr), so it should be safe to change the NS_ERROR_FAILURE to NS_OK.
(or at least quieten the NS_ENSURE_SUCCESS -- essentially, right now NS_ERROR_FAILURE is an expected case)
(Assignee)

Comment 9

10 years ago
(In reply to comment #7)

> 
> Absolutely nowhere else is the rv from ReadDBFolderInfo tested (verified using
> mxr), so it should be safe to change the NS_ERROR_FAILURE to NS_OK.
> 

I think it is safe compared to the previous behavior - but what I would like to understand is why we are calling it in cases where it does not work.
(Assignee)

Comment 10

10 years ago
I'm going to reopen this to keep it on the radar. I'm doing a lot of review right now of related code, and I want to consider what is the best approach to this as part of that review. I think for now we can leave the assertions as a temporary but harmless annoyance, but I'll get back to this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 11

10 years ago
Mark's checkin (comment #3) has the annoying side effect of expanding all of
the usenet account folders every time tb is started.  The pop3/imap folders
are not affected.

Anyone else seeing this?
(Assignee)

Comment 12

10 years ago
(In reply to comment #11)
> Mark's checkin (comment #3) has the annoying side effect of expanding all of
> the usenet account folders every time tb is started.  The pop3/imap folders
> are not affected.
> 
> Anyone else seeing this?
> 

I see this, and can confirm that backing out the patch removes the behavior. I'll look into it. Thanks for pointing this out.
(Assignee)

Comment 13

10 years ago
The news folder opening problem is occurring because of the "isServer" in the opening statement:

  if (!mNotifyCountChanges || mIsServer)

which was only present originally in the IMAP version. The mIsServer was added by bienvenu in 2001 as part of a patch dealing with a crash and some IMAP folder display problems. I don't understand the issues with that patch well enough to understand why it is there. I guess I could try to special-case the IMAP folder - but this was supposed to be a simplification, and I'm reluctant to start adding back in complications like that.

So I'm leaning now toward backing out the patch and abandoning the bug, unless David Bienvenu has some other suggestion.

Comment 14

10 years ago
worst case, you could remove the isServer check from the base class, override the method in nsImapMailFolder.cpp as follows:

return (mIsServer) ? NS_OK : nsMsgDBFolder::UpdateSummaryTotals(...)

I had a quick look at the bug with the patch that added the mIsServer check - I'd be a little leary of removing that check.
(Assignee)

Comment 15

10 years ago
Created attachment 328057 [details] [diff] [review]
isServer IMAP only, removed NS_ENSURE_SUCCESS

This is an addon patch. I moved the isServer check to the IMAP version, and changed the NS_ENSURE_SUCCESS to an if (NS_SUCCEEDED(...)) on the action section, and just returned the error.
Attachment #328057 - Flags: superreview?(bienvenu)
Attachment #328057 - Flags: review?(bienvenu)

Comment 16

10 years ago
Comment on attachment 328057 [details] [diff] [review]
isServer IMAP only, removed NS_ENSURE_SUCCESS

thx, Kent.

I usually don't like putting bug #'s in comments, but since cvs history isn't going to help us here so much, maybe you could add a comment in the nsImapMailFolder override about the bug that we were worried about -  bug 72871
Attachment #328057 - Flags: superreview?(bienvenu)
Attachment #328057 - Flags: superreview+
Attachment #328057 - Flags: review?(bienvenu)
Attachment #328057 - Flags: review+
(Assignee)

Comment 17

10 years ago
Created attachment 328062 [details] [diff] [review]
patch to checkin

Added comment referencing bug 72871.
Attachment #328057 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Checking in mailnews/base/util/nsMsgDBFolder.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgDBFolder.cpp,v  <--  nsMsgDBFolder.cpp
new revision: 1.352; previous revision: 1.351
done
Checking in mailnews/imap/src/nsImapMailFolder.cpp;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.cpp,v  <--  nsImapMailFolder.cpp
new revision: 1.826; previous revision: 1.825
done
Checking in mailnews/imap/src/nsImapMailFolder.h;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.h,v  <--  nsImapMailFolder.h
new revision: 1.256; previous revision: 1.255
done
Keywords: checkin-needed
(In reply to comment #11)
> Mark's checkin (comment #3) has the annoying side effect of expanding all of
> the usenet account folders every time tb is started.

SeaMonkey 2.0a1pre has/had this regression too.
(Assignee)

Updated

10 years ago
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.