Closed
Bug 420614
Opened 17 years ago
Closed 17 years ago
Drop nsAdapterEnumerator
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: standard8, Assigned: standard8)
References
()
Details
Attachments
(7 files, 5 obsolete files)
48.76 KB,
patch
|
standard8
:
review+
dmosedale
:
superreview+
|
Details | Diff | Splinter Review |
9.56 KB,
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
18.93 KB,
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
20.31 KB,
patch
|
Details | Diff | Splinter Review | |
12.35 KB,
patch
|
philor
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
12.24 KB,
patch
|
Details | Diff | Splinter Review | |
8.76 KB,
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
nsAdapterEnumerator (see http://mxr.mozilla.org/seamonkey/search?string=nsAdapterEnumerator) is a converter between nsIEnumerator and nsISimpleEnumerator. Used in a few places in the mailnews code base, we should probably just drop it, replacing the necessary code with nsISimpleEnumerator - otherwise its just an overhead and unnecessary complication.
Looking at the detail, it appears "nsIEnumerator nsIMsgFolder::GetSubFolder()" is the cause. Given the affect of changing this function (http://mxr.mozilla.org/seamonkey/search?string=GetSubFolders) I think the following plan may be best:
1) rename GetSubFolder to GetSubFolderObsolete.
2) implement "nsISimpleEnumerator nsIMsgFolder::GetSubFolder()"
3) switch blocks from the old function to the new in review-able bite-sized chuncks
4) drop the old functions and do the necessary tidy up.
Assignee | ||
Comment 1•17 years ago
|
||
This does more-or-less what I suggested in comment 0:
- Changes the current "nsIEnumerator GetSubFolders();" function to "[deprecated] readonly attribute nsIEnumerator subFoldersObsolete;"
- Creates a new "readonly attribute nsISimpleEnumerator subFolders"
- Drops nsAdapterEnumerator
- Does the necessary work to call get the correct attribute and handle the enumerator types.
I'm leaving getting rid of subFoldersObsolete to later patches in this bug. I considered it too much to rework all of it in one go.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attachment #310345 -
Flags: superreview?(dmose)
Attachment #310345 -
Flags: review?(neil)
Comment 2•17 years ago
|
||
Comment on attachment 310345 [details] [diff] [review]
Drop nsAdapterEnumerator v1
r- because of instances of GetSubFolders in platform-specific code (nsEudoraFilters line 371 was the first case that the compiler spotted.)
>+ // OBSOLETE, don't use this, use GetSubFolders instead
You mean subFolders, right ;-)
>- delete simpleEnumerator;
Eww, people deleting XPCOM objects :-(
(ok, so in this case it was never refcounted).
>+NS_IMETHODIMP nsImapMailFolder::GetSubFoldersObsolete(nsIEnumerator **aResult)
>+NS_IMETHODIMP nsImapMailFolder::GetSubFolders(nsISimpleEnumerator **aResult)
>+nsresult nsImapMailFolder::GetSubFoldersMain()
>+nsMsgLocalMailFolder::GetSubFoldersObsolete(nsIEnumerator* *result)
>+nsMsgLocalMailFolder::GetSubFolders(nsISimpleEnumerator **aResult)
>+nsMsgLocalMailFolder::GetSubFoldersMain()
Style nit: please move GetSubFolders to after GetSubFoldersMain, and put GetSubFoldersObsolete after GetSubFolders (i.e. reverse of your order).
>- rv =trashFolder->GetSubFolders(getter_AddRefs(aEnumerator));
>+ rv =trashFolder->GetSubFoldersObsolete(getter_AddRefs(aEnumerator));
Nit: worth adding a space in after the = ?
Attachment #310345 -
Flags: review?(neil) → review-
Assignee | ||
Comment 3•17 years ago
|
||
This should fix the compilation bustage and the nits.
Attachment #310345 -
Attachment is obsolete: true
Attachment #310526 -
Flags: superreview?(dmose)
Attachment #310526 -
Flags: review?(neil)
Attachment #310345 -
Flags: superreview?(dmose)
Comment 4•17 years ago
|
||
Comment on attachment 310526 [details] [diff] [review]
Drop nsAdapterEnumerator v2
Compiles OK.
The nits didn't win me quite as much patch niceness as I thought but then I realised you corrected the name of the out parameter :-)
Attachment #310526 -
Flags: review?(neil) → review+
Assignee | ||
Comment 5•17 years ago
|
||
Whilst fixing another bug, I noticed some warnings about functions being hidden by others. Turned out I'd missed the nsMsgNewsFolder changes. This patch fixes them in the same manner as the other nsMsg*Folder changes.
Attachment #310526 -
Attachment is obsolete: true
Attachment #311026 -
Flags: superreview?(dmose)
Attachment #311026 -
Flags: review+
Attachment #310526 -
Flags: superreview?(dmose)
Comment 6•17 years ago
|
||
Comment on attachment 311026 [details] [diff] [review]
[checked in] Drop nsAdapterEnumerator v3
Looks good! One nit:
>@@ -2852,7 +2834,7 @@ nsImapIncomingServer::GetNewMessagesForN
>
> // Loop through all subfolders to get new messages for them.
> nsCOMPtr<nsIEnumerator> aEnumerator;
>- retval = aFolder->GetSubFolders(getter_AddRefs(aEnumerator));
>+ retval = aFolder->GetSubFoldersObsolete(getter_AddRefs(aEnumerator));
As along as you're touching these lines (in this instance, as well as all the rest in this patch), how about changing the variable name to something that doesn't start with a, since these are not, in fact, arguments.
sr=dmose. Bonus points for writing a simple |make check| unit test for nsIMsgFolder.subFolders(), though I'll understand if that entrains too much extra work.
Attachment #311026 -
Flags: superreview?(dmose) → superreview+
Assignee | ||
Comment 7•17 years ago
|
||
Comment on attachment 311026 [details] [diff] [review]
[checked in] Drop nsAdapterEnumerator v3
I checked this in yesterday with the variable names changed as per Dan's comment.
Attachment #311026 -
Attachment description: Drop nsAdapterEnumerator v3 → [checked in] Drop nsAdapterEnumerator v3
Assignee | ||
Comment 8•17 years ago
|
||
Round 1 of the subFoldersObsolete cleanup. Including some of cleanup as most of that file really looks bad.
Attachment #313788 -
Flags: superreview?(dmose)
Attachment #313788 -
Flags: review?(neil)
Comment 9•17 years ago
|
||
Comment on attachment 313788 [details] [diff] [review]
[checked in] Drop subFoldersObsolete from nsImapMailFolder.cpp v1
> parentWindow = do_QueryInterface(docShell);
This won't work ;-) Perhaps they meant do_GetInterface instead?
Attachment #313788 -
Flags: review?(neil) → review+
Assignee | ||
Comment 10•17 years ago
|
||
(In reply to comment #9)
> (From update of attachment 313788 [details] [diff] [review])
> > parentWindow = do_QueryInterface(docShell);
> This won't work ;-) Perhaps they meant do_GetInterface instead?
>
I'm going to separate fixing that into a separate bug.
Assignee | ||
Comment 11•17 years ago
|
||
Comment on attachment 313788 [details] [diff] [review]
[checked in] Drop subFoldersObsolete from nsImapMailFolder.cpp v1
Neil, mind sr'ing this as well?
Attachment #313788 -
Flags: superreview?(dmose) → superreview?(neil)
Comment 12•17 years ago
|
||
Comment on attachment 313788 [details] [diff] [review]
[checked in] Drop subFoldersObsolete from nsImapMailFolder.cpp v1
>+ nsCOMPtr<nsISimpleEnumerator> enumerator;
>+ rv = trashFolder->GetSubFolders(getter_AddRefs(enumerator));
>+ NS_ENSURE_SUCCESS(rv, rv);
Maybe move this inside the confirmDeletion block?
Attachment #313788 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Comment 13•17 years ago
|
||
Comment on attachment 313788 [details] [diff] [review]
[checked in] Drop subFoldersObsolete from nsImapMailFolder.cpp v1
Checked in
Checking in mailnews/imap/src/nsImapMailFolder.cpp;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.cpp,v <-- nsImapMailFolder.cpp
new revision: 1.805; previous revision: 1.804
For testers, check imap functions for delete/rename on folders works. Note: compare also with builds previous to the one this is in as I believe there are other bugs in this area as well.
Attachment #313788 -
Attachment description: Drop subFoldersObsolete from nsImapMailFolder.cpp v1 → [checked in] Drop subFoldersObsolete from nsImapMailFolder.cpp v1
Comment 14•17 years ago
|
||
Could this all have something to do with bug 428729?
The checkin here from 2008-04-16 04:49 falls in that bug's regression range (see bug 420614#1 and bug 420614#2)
Assignee | ||
Comment 15•17 years ago
|
||
(In reply to comment #14)
> Could this all have something to do with bug 428729?
>
> The checkin here from 2008-04-16 04:49 falls in that bug's regression range
> (see bug 420614#1 and bug 420614#2)
That is highly unlikely considering that this was a change affecting imap and how it gets its sub folders.
Assignee | ||
Comment 16•17 years ago
|
||
Attachment #316673 -
Flags: review?(neil)
Comment 17•17 years ago
|
||
It seems (not 100% sure yet, need to pinpoint the nightly) that the changes here cause GetSubFolders() to silently kill JS code - at least the code in my removedupes extension, when calling GetSubFolders() for non-leaf folders. Should I file a separate bug? Because I don't really follow many of the comments here.
Assignee | ||
Comment 18•17 years ago
|
||
(In reply to comment #17)
> It seems (not 100% sure yet, need to pinpoint the nightly) that the changes
> here cause GetSubFolders() to silently kill JS code - at least the code in my
> removedupes extension, when calling GetSubFolders() for non-leaf folders.
> Should I file a separate bug? Because I don't really follow many of the
> comments here.
I have just tried your extension. When I try to invoke your extension I get:
Error: folder.GetSubFolders is not a function
Source File: chrome://removedupes/content/removedupes.js
Line: 264
on the error console. I don't see a problem with this, I think the message is sufficient.
Comment 19•17 years ago
|
||
Comment on attachment 316673 [details] [diff] [review]
Finish replacing subFoldersObsolete in cpp files
Awaiting a new patch with minor fixes and -w as requested on IRC ;-)
Attachment #316673 -
Flags: review?(neil)
Assignee | ||
Comment 20•17 years ago
|
||
Updated patch per irc discussion.
Attachment #316673 -
Attachment is obsolete: true
Attachment #317006 -
Flags: superreview?(neil)
Attachment #317006 -
Flags: review?(neil)
Assignee | ||
Comment 21•17 years ago
|
||
Comment 22•17 years ago
|
||
Comment on attachment 317006 [details] [diff] [review]
[normal version checked in] Finish replacing subFoldersObsolete in cpp files v2 (diff -w)
These notes are possibly beyond the scope of this bug.
> NS_IMETHODIMP nsMsgDBFolder::WriteToFolderCache(nsIMsgFolderCache *folderCache, PRBool deep)
There seem to be lots of null-checks of folderCache when one would do...
>+ nsCOMPtr<nsIRDFResource> folderResource(do_QueryInterface(item));
>+ nsCOMPtr<nsIMsgFolder> folder(do_QueryInterface(item));
> if (folderResource && folder)
> {
> const char *folderURI;
Seems wasteful to ask the resource for the URI that the folder already knows!
Attachment #317006 -
Flags: superreview?(neil)
Attachment #317006 -
Flags: superreview+
Attachment #317006 -
Flags: review?(neil)
Attachment #317006 -
Flags: review+
Assignee | ||
Updated•17 years ago
|
Attachment #317006 -
Attachment description: Finish replacing subFoldersObsolete in cpp files v2 (diff -w) → [checked in] Finish replacing subFoldersObsolete in cpp files v2 (diff -w)
Assignee | ||
Updated•17 years ago
|
Attachment #317007 -
Attachment description: Finish replacing subFoldersObsolete in cpp files v2 (normal patch) → [checkin in] Finish replacing subFoldersObsolete in cpp files v2 (normal patch)
Assignee | ||
Comment 23•17 years ago
|
||
Comment on attachment 317006 [details] [diff] [review]
[normal version checked in] Finish replacing subFoldersObsolete in cpp files v2 (diff -w)
Note to testers: check folder related actions/displays work ok. New messages, rename etc etc.
Attachment #317006 -
Attachment description: [checked in] Finish replacing subFoldersObsolete in cpp files v2 (diff -w) → [normal version checked in] Finish replacing subFoldersObsolete in cpp files v2 (diff -w)
Assignee | ||
Comment 24•17 years ago
|
||
Finishes removing the js uses of subFoldersObsolete. I'll attach the backend removal in another patch.
Attachment #317511 -
Flags: superreview?(neil)
Attachment #317511 -
Flags: review?(philringnalda)
Assignee | ||
Comment 25•17 years ago
|
||
Assignee | ||
Comment 26•17 years ago
|
||
Comment on attachment 317511 [details] [diff] [review]
Finish replacing subFoldersObsolete in js files (diff -w)
Unfortunately, I missed testing of SearchDialog.js :-(
Attachment #317511 -
Attachment is obsolete: true
Attachment #317511 -
Flags: superreview?(neil)
Attachment #317511 -
Flags: review?(philringnalda)
Assignee | ||
Updated•17 years ago
|
Attachment #317512 -
Attachment is obsolete: true
Assignee | ||
Comment 27•17 years ago
|
||
Revised version should be simpler now as well.
Attachment #317572 -
Flags: superreview?(neil)
Attachment #317572 -
Flags: review?(philringnalda)
Assignee | ||
Comment 28•17 years ago
|
||
Assignee | ||
Updated•17 years ago
|
Attachment #317572 -
Attachment description: Finish replacing subFoldersObsolete in js files (diff -w) → Finish replacing subFoldersObsolete in js files (diff -w) v2
Assignee | ||
Comment 29•17 years ago
|
||
Once the js patch is in, this will remove the rest.
Attachment #317581 -
Flags: superreview?(neil)
Attachment #317581 -
Flags: review?(neil)
Updated•17 years ago
|
Attachment #317572 -
Flags: superreview?(neil) → superreview+
Updated•17 years ago
|
Attachment #317572 -
Flags: review?(philringnalda) → review+
Comment 30•17 years ago
|
||
Comment on attachment 317581 [details] [diff] [review]
[checked in] Remove subFoldersObsolete function.
> // XXX When GetSubFoldersObsolete goes away (bug 420614), this can be merged
> // into GetSubFolders (see also note at return statement).
I think this comment can go too ;-)
Attachment #317581 -
Flags: superreview?(neil)
Attachment #317581 -
Flags: superreview+
Attachment #317581 -
Flags: review?(neil)
Attachment #317581 -
Flags: review+
Assignee | ||
Updated•17 years ago
|
Attachment #317572 -
Attachment description: Finish replacing subFoldersObsolete in js files (diff -w) v2 → [normal version checked in] Finish replacing subFoldersObsolete in js files (diff -w) v2
Assignee | ||
Updated•17 years ago
|
Attachment #317573 -
Attachment description: Finish replacing subFoldersObsolete in js files (normal patch) v2 → [checked in] Finish replacing subFoldersObsolete in js files (normal patch) v2
Assignee | ||
Updated•17 years ago
|
Attachment #317581 -
Attachment description: Remove subFoldersObsolete function. → [checked in] Remove subFoldersObsolete function.
Assignee | ||
Comment 31•17 years ago
|
||
This is now all checked in so marking as fixed. For testers, look for any regressions to do with folder selection/list display and related things.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•