Last Comment Bug 834020 - remove use of nsIEnumerator in mailnews C++ code
: remove use of nsIEnumerator in mailnews C++ code
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 21.0
Assigned To: :aceman
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 436089 833988
Blocks: 834911
  Show dependency treegraph
 
Reported: 2013-01-23 15:01 PST by :aceman
Modified: 2013-02-07 09:47 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (10.05 KB, patch)
2013-01-23 15:31 PST, :aceman
neil: review+
rkent: feedback+
Details | Diff | Splinter Review
patch v2 (9.34 KB, patch)
2013-01-25 14:14 PST, :aceman
acelists: review+
Details | Diff | Splinter Review

Description :aceman 2013-01-23 15:01:42 PST
nsIEnumerator seems obsolete per https://developer.mozilla.org/en-US/docs/XPCOM_array_guide.

Looks like there are only a few occurrences left:

/mailnews/addrbook/src/nsAddrDatabase.cpp (View Hg log or Hg annotations)

    line 2697 -- // nsIEnumerator methods:

/mailnews/addrbook/src/nsAddrDatabase.h (View Hg log or Hg annotations)

    line 15 -- #include "nsIEnumerator.h"
    line 337 -- nsresult CreateCardsForMailList(nsIMdbRow *pListRow, nsIEnumerator **result);

/mailnews/base/src/nsMessengerOSXIntegration.mm (View Hg log or Hg annotations)

    line 681 -- nsCOMPtr<nsIEnumerator> enumerator;

/mailnews/base/src/nsMessengerWinIntegration.cpp (View Hg log or Hg annotations)

    line 790 -- nsCOMPtr<nsIEnumerator> enumerator;

/mailnews/base/util/iteratorUtils.jsm (View Hg log or Hg annotations)

    line 53 -- * nsIEnumerator
    line 101 -- // Now try nsIEnumerator
    line 102 -- if (aEnum instanceof Ci.nsIEnumerator) {

/mailnews/base/util/nsMsgUtils.h (View Hg log or Hg annotations)

    line 11 -- #include "nsIEnumerator.h"

/mailnews/compose/src/nsMsgCompose.cpp (View Hg log or Hg annotations)

    line 4690 -- nsCOMPtr<nsIEnumerator> enumerator;

/mailnews/db/msgdb/src/nsMsgDatabase.cpp (View Hg log or Hg annotations)

    line 18 -- #include "nsIEnumerator.h"

/mailnews/imap/src/nsImapMailFolder.cpp (View Hg log or Hg annotations)

    line 14 -- #include "nsIEnumerator.h"

/mailnews/imap/src/nsImapOfflineSync.h (View Hg log or Hg annotations)

    line 17 -- #include "nsIEnumerator.h"
    line 67 -- nsCOMPtr <nsIEnumerator> m_serverEnumerator;

/mailnews/import/src/nsImportService.cpp (View Hg log or Hg annotations)

    line 19 -- #include "nsIEnumerator.h"

/mailnews/import/src/nsImportService.h (View Hg log or Hg annotations)

    line 15 -- #include "nsIEnumerator.h"

/mailnews/local/src/nsLocalMailFolder.cpp (View Hg log or Hg annotations)

    line 21 -- #include "nsIEnumerator.h"

/mailnews/news/src/nsNewsDownloader.h (View Hg log or Hg annotations)

    line 17 -- #include "nsIEnumerator.h"
    line 119 -- nsCOMPtr <nsIEnumerator> m_serverEnumerator;

/mailnews/news/src/nsNewsFolder.cpp (View Hg log or Hg annotations)

    line 55 -- #include "nsIEnumerator.h"

Some of them get naturally away via the nsISupportsArray conversions in other bugs.
Comment 1 :aceman 2013-01-23 15:22:48 PST
Looks like many of those are just stray leftover includes.
One open topic I see is if we can remove support for nsIEnumerator from /mailnews/base/util/iteratorUtils.jsm too. Or do we leave it in for some time until extensions stop using it too? Can anybody check if any TB/SM extensions are using it?
Comment 2 :aceman 2013-01-23 15:31:53 PST
Created attachment 705609 [details] [diff] [review]
patch

After applying patches in bug 436089 and bug 833988 and this patch, I see no traces of nsIEnumerator left in c-c. What do you think?
Comment 3 :aceman 2013-01-23 15:42:00 PST
(In reply to :aceman from comment #1)
> Looks like many of those are just stray leftover includes.
> One open topic I see is if we can remove support for nsIEnumerator from
> /mailnews/base/util/iteratorUtils.jsm too. 
Ok, I think we should leave it in until we kill nsISupportsArray first, as there are still some spots where we call .enumerate() on some arrays (in JS) and those could produce nsIEnumerator if an array was nsISupportsArray. Or is somebody up to review what type those arrays are? :)
Comment 4 neil@parkwaycc.co.uk 2013-01-24 04:13:12 PST
Do I need a patch for nsImapOfflineSync.cpp or is my tree out of date?
Comment 5 :aceman 2013-01-24 05:15:31 PST
"patches in bug 436089 and bug 833988 and this patch" applied in that order worked for me yesterday. Bug 436089 changes nsImapOfflineSync.cpp.
Comment 6 neil@parkwaycc.co.uk 2013-01-24 05:23:13 PST
(In reply to aceman from comment #5)
> "patches in bug 436089 and bug 833988 and this patch" applied in that order
> worked for me yesterday. Bug 436089 changes nsImapOfflineSync.cpp.

Actually I guess I only need the C++ attachments to compile this patch.
Comment 7 :aceman 2013-01-24 05:27:25 PST
Yes, if you do not need to run the app or mozmill tests.
Comment 8 neil@parkwaycc.co.uk 2013-01-24 05:30:13 PST
Comment on attachment 705609 [details] [diff] [review]
patch

r=me pending f=rkent on the JS enumerator change.
Comment 9 Kent James (:rkent) 2013-01-25 12:59:15 PST
Comment on attachment 705609 [details] [diff] [review]
patch

createCardsForMailList in not xpcom, so it does not show in addons.

The only uses that I see of nsIEnumerator in mail-related addons are of the nature:

else if (iter instanceof Components.interfaces.nsIEnumerator) { // TB 2

or otherwise checks for old versions of TB. As long as the underlying interface continues to exist so that Components.interfaces.nsIEnumerator doesn't fail, then they should be fine.
Comment 10 :aceman 2013-01-25 14:05:14 PST
I remove CreateCardsForMailList because it looks like an unused define (only in .h, but no implementation).

I understand what you are saying. But I think there may be hidden usages of nsIEnumerator in core and in extensions (e.g. nsISupportsArray.enumerate()).
So I'll leave the support in iteratorUtils.jsm for now. I make a new bug to remove it after we remove more of nsISupportsArray from c-c.
Comment 11 :aceman 2013-01-25 14:14:13 PST
Actually I think we need to keep it until all of nsISupportsArray and nsIEnumerator are removed from c-c AND m-c (which may be harder).
Comment 12 :aceman 2013-01-25 14:14:34 PST
Created attachment 706595 [details] [diff] [review]
patch v2
Comment 13 :aceman 2013-01-25 14:16:57 PST
Can't checkin until bug 436089 lands :)
Comment 14 Ryan VanderMeulen [:RyanVM] 2013-02-06 06:39:29 PST
https://hg.mozilla.org/comm-central/rev/124128336484
Comment 15 Ryan VanderMeulen [:RyanVM] 2013-02-06 07:43:00 PST
Backed out for build bustage.
https://hg.mozilla.org/comm-central/rev/c0d255b60629

https://tbpl.mozilla.org/php/getParsedLog.php?id=19494063&tree=Thunderbird-Trunk

In file included from ../../../../mailnews/news/src/nsNntpService.cpp:41:0:
../../../../mailnews/news/src/nsNewsDownloader.h:118:13: error: 'nsIEnumerator' was not declared in this scope
../../../../mailnews/news/src/nsNewsDownloader.h:118:26: error: template argument 1 is invalid
Comment 16 :aceman 2013-02-06 07:48:23 PST
Yes, this depends on bug 436089 that was backed out right now and you checked it in before I could remove the checkin-needed :)
Sorry about that.
Comment 17 Ryan VanderMeulen [:RyanVM] 2013-02-07 09:47:40 PST
https://hg.mozilla.org/comm-central/rev/fa45b123a36a

Note You need to log in before you can comment on or make changes to this bug.