Closed
Bug 473458
Opened 15 years ago
Closed 15 years ago
nsIMsgFolder::GetMessages and nsMsgDBFolder::GetDatabase don't need nsIMsgWindow arguments
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b2
People
(Reporter: standard8, Assigned: standard8)
References
Details
(Keywords: memory-footprint)
Attachments
(2 files, 2 obsolete files)
17.81 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
71.07 KB,
patch
|
standard8
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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)
Comment 1•15 years ago
|
||
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.
Assignee | ||
Comment 2•15 years ago
|
||
(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).
Comment 3•15 years ago
|
||
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.
Updated•15 years ago
|
Attachment #356836 -
Flags: superreview?(bienvenu)
Attachment #356836 -
Flags: superreview+
Attachment #356836 -
Flags: review?(bienvenu)
Attachment #356836 -
Flags: review+
Assignee | ||
Comment 4•15 years ago
|
||
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
Assignee | ||
Comment 5•15 years ago
|
||
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)
Assignee | ||
Comment 6•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #357651 -
Flags: review?(neil) → review+
Comment 7•15 years ago
|
||
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...
Assignee | ||
Comment 8•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #357702 -
Flags: superreview?(bienvenu) → superreview+
Comment 9•15 years ago
|
||
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.
Assignee | ||
Comment 10•15 years ago
|
||
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: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b2
Comment 11•15 years ago
|
||
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
Comment 12•15 years ago
|
||
I'm seeing that as well - Valery, do you have reason to suspect the patch in this bug?
Comment 13•15 years ago
|
||
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.
Description
•