Closed Bug 473458 Opened 11 years ago Closed 11 years ago

nsIMsgFolder::GetMessages and nsMsgDBFolder::GetDatabase don't need nsIMsgWindow arguments

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b2

People

(Reporter: standard8, Assigned: standard8)

Details

(Keywords: memory-footprint)

Attachments

(2 files, 2 obsolete files)

I was looking at the send later code and I was wondering why it needed a nsIMsgWindow. After a bit of digging, I realised that the various GetDatabase() functions in the nsMsgDBFolder derived classes don't need an nsIMsgWindow argument, and therefore neither does nsIMsgFolder::GetMessages.

nsIMsgFolder::GetMsgDatabase also seems a candidate for this removal.

The first patch I'm attaching drops the argument from nsMsgDBFolder::GetDatabase, which is quite simple as its a c++ only function.
Attachment #356836 - Flags: superreview?(bienvenu)
Attachment #356836 - Flags: review?(bienvenu)
Although I will admit that I routinely setup js that sends null to arguments like this, I think that the logic is that opening a database can sometimes result in problems of which the user should be notified. There are not many examples of this, but one is http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#754 That is done one level higher that the getDatabase call, but there could be some logic to moving some of that functionality into getDatabase at some point in the near future. Since I plan to work on the database in the next couple of weeks to try to reduce the cases where the database is needlessly blown away, there is a chance I will regret this change, wishing I could send an error message during opening. That is unlikely, but possible.

So I have mixed feelings about this change.
(In reply to comment #1)
> Although I will admit that I routinely setup js that sends null to arguments
> like this, I think that the logic is that opening a database can sometimes
> result in problems of which the user should be notified. There are not many
> examples of this, but one is
> http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#754

IMHO that's a reasonable example of how not to do it. Firstly, the only way the caller can control if the message is displayed or not is by passing in a message window - which they may want to provide e.g. for PromptForCachePassword a bit further down.

I would have really expected that we'd propagate the error back to the caller and let them decide how to handle it.

> That is done one level higher that the getDatabase call, but there could be
> some logic to moving some of that functionality into getDatabase at some point
> in the near future.

We shouldn't be relying on a message window to display errors especially when there may be no message window (e.g. background sync or send operation) - how would the user get notified of errors?

My issue with GetDatabase and friends is that no-one seems to know when to call with a message window and when to call without, which isn't surprising seeing as you don't actually need to.

I caught this as part of the code I'm starting to look at for sending emails in the background - that most likely won't have a message window to send to GetDatabase as it will be a service running in the background.

> Since I plan to work on the database in the next couple of
> weeks to try to reduce the cases where the database is needlessly blown away,
> there is a chance I will regret this change, wishing I could send an error
> message during opening. That is unlikely, but possible.

Well I can pass it just null for now, but I'd really like to get rid of this confusion as I don't see a need for it (and it makes at least two parts of nsIMsgFolder more complicated than necessary).
OK I'm a hypocrite, on the patch I'm working on now a message window was not easily available, so I just punted and used a null.

So go ahead and make this change. I remove my objections.
Attachment #356836 - Flags: superreview?(bienvenu)
Attachment #356836 - Flags: superreview+
Attachment #356836 - Flags: review?(bienvenu)
Attachment #356836 - Flags: review+
Comment on attachment 356836 [details] [diff] [review]
[checked in] Part 1 - Drop argument from nsMsgDBFolder::GetDatabase

http://hg.mozilla.org/comm-central/rev/10d6c4927ad2
Attachment #356836 - Attachment description: Part 1 - Drop argument from nsMsgDBFolder::GetDatabase → [checked in] Part 1 - Drop argument from nsMsgDBFolder::GetDatabase
Drops the nsIMsgWindow argument from getMessages and getMsgDatabase, as a result both of these can be turned into attributes - I've gone for:

readonly attribute nsISimpleEnumerator messages;
attribute nsIMsgDatabase msgDatabase;

Makes them much easier to access from javascript and saves the unnecessary parameter passing.
Attachment #357650 - Flags: superreview?(bienvenu)
Attachment #357650 - Flags: review?(neil)
Opps, I forgot to actually check the unit test output - this fixes the nsNewsFolder version of GetMessages that I had missed.
Attachment #357650 - Attachment is obsolete: true
Attachment #357651 - Flags: superreview?(bienvenu)
Attachment #357651 - Flags: review?(neil)
Attachment #357650 - Flags: superreview?(bienvenu)
Attachment #357650 - Flags: review?(neil)
Attachment #357651 - Flags: review?(neil) → review+
Comment on attachment 357651 [details] [diff] [review]
Part 2 - Drop argument from nsIMsgFolder's getMessages and getMsgDatabase functions

>+  var msgdb = msgFolder.msgDatabase();
msgFolder.msgDatabase;

>+     rv=folder->GetMsgDatabase(getter_AddRefs(db));
Nit: rv = folder

I can't get the patch to apply though...
Addressed Neil's comments, this needs the latest sources to apply.
Attachment #357651 - Attachment is obsolete: true
Attachment #357702 - Flags: superreview?(bienvenu)
Attachment #357702 - Flags: review+
Attachment #357651 - Flags: superreview?(bienvenu)
Attachment #357702 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 357702 [details] [diff] [review]
Part 2 - Drop argument from nsIMsgFolder's getMessages and getMsgDatabase functions v2

nice cleanup - might be worth posting to the tb dev newsgroup since this will probably affect a fair number of extensions.
Checked in: http://hg.mozilla.org/comm-central/rev/649e4c6a0e38

Note to testers: this affects a large part of mailnews - test anything that is interacting with folders.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b2
I see problems with refreshing of Saved Search in the latest nightly: Saved Search folder with new messages appears as not having new messages (not bold) until I select it. After I select the Saved Search with my mouse it's content gets refreshed and this folder becomes bold. 
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090125 Shredder/3.0b2pre
I'm seeing that as well - Valery, do you have reason to suspect the patch in this bug?
There are no special reasons to suspect this patch... I just have seen the Mark Banner warning: "this affects a large part of mailnews - test anything that is interacting with folders". The other reason: it seems the issue with 'non-refreshing' Saved Searches become worse in the last 2-3 days... (As well as Junk mail auto-filtering - but as far as I understand this is not the current patch problem).
You need to log in before you can comment on or make changes to this bug.