Closed Bug 420614 Opened 17 years ago Closed 17 years ago

Drop nsAdapterEnumerator

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

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+
Details | Diff | Splinter Review
18.93 KB, patch
neil
: review+
Details | Diff | Splinter Review
20.31 KB, patch
Details | Diff | Splinter Review
12.35 KB, patch
philor
: review+
Details | Diff | Splinter Review
12.24 KB, patch
Details | Diff | Splinter Review
8.76 KB, patch
neil
: review+
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.
Attached patch Drop nsAdapterEnumerator v1 (obsolete) — Splinter Review
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 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-
Attached patch Drop nsAdapterEnumerator v2 (obsolete) — Splinter Review
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 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+
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 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+
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
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 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+
(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.
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 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+
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
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)
(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.
Attachment #316673 - Flags: review?(neil)
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.
(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 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)
Updated patch per irc discussion.
Attachment #316673 - Attachment is obsolete: true
Attachment #317006 - Flags: superreview?(neil)
Attachment #317006 - Flags: review?(neil)
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+
Attachment #317006 - Attachment description: Finish replacing subFoldersObsolete in cpp files v2 (diff -w) → [checked in] Finish replacing subFoldersObsolete in cpp files v2 (diff -w)
Attachment #317007 - Attachment description: Finish replacing subFoldersObsolete in cpp files v2 (normal patch) → [checkin in] Finish replacing subFoldersObsolete in cpp files v2 (normal patch)
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)
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)
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)
Attachment #317512 - Attachment is obsolete: true
Revised version should be simpler now as well.
Attachment #317572 - Flags: superreview?(neil)
Attachment #317572 - Flags: review?(philringnalda)
Attachment #317572 - Attachment description: Finish replacing subFoldersObsolete in js files (diff -w) → Finish replacing subFoldersObsolete in js files (diff -w) v2
Once the js patch is in, this will remove the rest.
Attachment #317581 - Flags: superreview?(neil)
Attachment #317581 - Flags: review?(neil)
Attachment #317572 - Flags: superreview?(neil) → superreview+
Attachment #317572 - Flags: review?(philringnalda) → review+
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+
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
Attachment #317573 - Attachment description: Finish replacing subFoldersObsolete in js files (normal patch) v2 → [checked in] Finish replacing subFoldersObsolete in js files (normal patch) v2
Attachment #317581 - Attachment description: Remove subFoldersObsolete function. → [checked in] Remove subFoldersObsolete function.
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
Depends on: 437848
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: