remove use of nsIEnumerator in mailnews C++ code

RESOLVED FIXED in Thunderbird 21.0

Status

MailNews Core
Backend
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 21.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

9.34 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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?
(Assignee)

Comment 2

4 years ago
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?
Attachment #705609 - Flags: review?(neil)
Attachment #705609 - Flags: feedback?(kent)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
(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

Comment 4

4 years ago
Do I need a patch for nsImapOfflineSync.cpp or is my tree out of date?
(Assignee)

Comment 5

4 years ago
"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

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
Yes, if you do not need to run the app or mozmill tests.

Comment 8

4 years ago
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 9

4 years ago
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+
(Assignee)

Comment 10

4 years ago
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
(Assignee)

Comment 11

4 years ago
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).
(Assignee)

Comment 12

4 years ago
Created attachment 706595 [details] [diff] [review]
patch v2
Attachment #705609 - Attachment is obsolete: true
Attachment #706595 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 13

4 years ago
Can't checkin until bug 436089 lands :)
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Blocks: 834911
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/124128336484
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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 → ---
(Assignee)

Comment 16

4 years ago
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
Last Resolved: 4 years ago4 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.