Closed Bug 834020 Opened 7 years ago Closed 7 years ago

remove use of nsIEnumerator in mailnews C++ code

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 21.0

People

(Reporter: aceman, Assigned: aceman)

References

()

Details

Attachments

(1 file, 1 obsolete file)

9.34 KB, patch
aceman
: review+
Details | Diff | Splinter Review
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.
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?
Attached patch patch (obsolete) — Splinter Review
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?
Attachment #705609 - Flags: review?(neil)
Attachment #705609 - Flags: feedback?(kent)
Status: NEW → ASSIGNED
(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? :)
Depends on: 394167
Do I need a patch for nsImapOfflineSync.cpp or is my tree out of date?
"patches in bug 436089 and bug 833988 and this patch" applied in that order worked for me yesterday. Bug 436089 changes nsImapOfflineSync.cpp.
(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.
Yes, if you do not need to run the app or mozmill tests.
Comment on attachment 705609 [details] [diff] [review]
patch

r=me pending f=rkent on the JS enumerator change.
Attachment #705609 - Flags: review?(neil) → review+
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.
Attachment #705609 - Flags: feedback?(kent) → feedback+
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.
No longer depends on: 394167
Summary: remove use of nsIEnumerator in mailnews → remove use of nsIEnumerator in mailnews C++ code
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).
Attached patch patch v2Splinter Review
Attachment #705609 - Attachment is obsolete: true
Attachment #706595 - Flags: review+
Keywords: checkin-needed
Can't checkin until bug 436089 lands :)
Keywords: checkin-needed
Blocks: 834911
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/124128336484
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 21.0
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 21.0 → ---
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.
https://hg.mozilla.org/comm-central/rev/fa45b123a36a
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 21.0
You need to log in before you can comment on or make changes to this bug.